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

(Do not merge) [ICE] Implemented Kjeldoran Guard #8593

Closed
wants to merge 3 commits into from

Conversation

Alex-Vasile
Copy link
Contributor

No description provided.

@sprangg
Copy link
Contributor

sprangg commented Jan 25, 2022

Thank you so much! I have a Pre-Mirage Masters set project and I've been longing to get at least Kjeldoran Elite Guard in! Glad to see someone giving love to the old sets.

Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranEliteGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranGuard.java Outdated Show resolved Hide resolved
Mage.Sets/src/mage/cards/k/KjeldoranGuard.java Outdated Show resolved Hide resolved
@theelk801
Copy link
Contributor

I'm gonna hold off on merging this until I can get a more conclusive answer with regards to how "defending player" is determined but otherwise this looks good to me. We've got NEO previews starting tomorrow so there's gonna be plenty of easy stuff to help implement. Thanks for the contribution.

@Alex-Vasile
Copy link
Contributor Author

Alex-Vasile commented Jan 26, 2022

Sounds good! Based on rereading it, I would assume it refers to whoever is being attacked in a round of combat, regardless of which player's gets targeted by the ability.

EDIT: While you get clarification, I would assume Arcum's Sleigh would share the same interpretation?

@jeffwadsworth
Copy link
Contributor

Yeah, the "defending player"...this card should be removed because of ambiguity. Or reworded to make some sense. For example, the target creature attacked the "defending player", etc.

@awjackson
Copy link
Contributor

awjackson commented Jan 26, 2022

I'm sure that the intention of Kjeldoran Guard is that you're supposed to be able to use it either on offense or defense (to pump one of your blocking creatures), and if you use it on defense it cares about you controlling snow lands. That's unambiguously what the card's wording means in a two-player game, where "defending player" is defined by 506.2 and multiplayer-specific rules don't apply.

Compare it to Snowblind, which (explicitly) uses the defending player's snow lands when the enchanted creature is attacking and the controller's snow lands the rest of the time.

However, apparently there was a big discussion last year over Arcum's Sleigh which seems never to have gotten resolved, which means that even the MTG Rules Manager can't tell you how these cards work or whether they even work at all under the current rules.

https://articles.starcitygames.com/select/snow-battles-a-new-way-to-play-commander/
https://twitter.com/SheldonMenery/status/1360024520527077389

@Alex-Vasile
Copy link
Contributor Author

Those are from over a year ago and still no resolution. Should I remove Kjeldoran Guard out of this PR, so that we can at least merge Kjeldoran Elite Guard in, and open a separate one for Kjeldoran Guard?

@Alex-Vasile Alex-Vasile changed the title [ICE] Implemented Kjeldoran Guard and Kjeldoran Elite Guard (Do not merge) [ICE] Implemented Kjeldoran Guard Jan 26, 2022
@Alex-Vasile Alex-Vasile marked this pull request as draft January 27, 2022 19:42
@Susucre
Copy link
Contributor

Susucre commented Aug 1, 2023

From the Multiplayer section of the rules:

802.2. As the combat phase starts, the attacking player doesn’t choose an opponent to become the defending player. Instead, all the attacking player’s opponents are defending players during the combat phase.

I think for that old card (and a few other, like [[Blaze of Glory]]), defending player means "opponent of the attacking player", limited to the combat phase.

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Blaze of Glory - (Gatherer) (Scryfall) (EDHREC)

{W}
Instant
Cast this spell only during combat before blockers are declared.
Target creature defending player controls can block any number of creatures this turn. It blocks each attacking creature this turn if able.

this.subtype.add(SubType.SOLDIER);
this.power = new MageInt(1);
this.toughness = new MageInt(1);

Copy link
Contributor

Choose a reason for hiding this comment

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

Full text is:

// {T}: Target creature gets +1/+1 until end of turn. When that creature leaves the battlefield this turn, sacrifice Kjeldoran Guard. Activate only during combat and only if defending player controls no snow lands.

// When that creature leaves the battlefield this turn, sacrifice Kjeldoran Guard.
// Activate only during combat and only if defending player controls no snow lands.
CompoundCondition snowLandAndCombatCondition = new CompoundCondition(
new InvertCondition(new DefendingPlayerControlsCondition(snowLandFiler)),
Copy link
Contributor

Choose a reason for hiding this comment

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

replace the condition with one using the following predicate (in addition to checking for the permanent to be a snow land permanent): DefendingPlayerControlsNoSourcePredicate.instance

}

@Override
public String getRule() { return "that creature left the battlefield this turn"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something like "When that creature left the battlefield, sacrifice {this}." would be a little nicer on the delayed trigger message.

It could be done by setting setTriggerPhrase in the constructor for instance.

@Susucre
Copy link
Contributor

Susucre commented Aug 4, 2023

@xenohedron With the age of this PR, and without knowing if OP is still active, should I take over and make my own implem?

@xenohedron
Copy link
Contributor

xenohedron commented Aug 4, 2023

Yeah, for any card implementation PR that's more than a couple months old, it's fine to make your own fresh one if adjustments are needed.

(Do link the previous work for reference.)

@Alex-Vasile
Copy link
Contributor Author

@xenohedron With the age of this PR, and without knowing if OP is still active, should I take over and make my own implem?

I am not active anymore, feel free to re-implement from scratch or to copy this to a new PR and make the changes you need.

@Susucre
Copy link
Contributor

Susucre commented Aug 4, 2023

Thanks, sorry for the pings then!

@Alex-Vasile
Copy link
Contributor Author

Thanks, sorry for the pings then!

No worries, I left them on for the PRs specifically for reasons like this.

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

7 participants