-
-
Notifications
You must be signed in to change notification settings - Fork 148
Partial: Doom's Time Platform #7022
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
Open
JacobWoodson
wants to merge
2
commits into
phase-rs:main
Choose a base branch
from
JacobWoodson:card/dooms-time-platform
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
119 changes: 119 additions & 0 deletions
119
crates/engine/tests/integration/doom_s_time_platform_exile_with_time_counters.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| //! Doom's Time Platform: "Whenever you attack, exile target nonland card from | ||
| //! your graveyard with two time counters on it. If it doesn't have suspend, it | ||
| //! gains suspend." | ||
| //! | ||
| //! Pre-fix, "with two time counters on it" was consumed by `parse_target` as a | ||
| //! `FilterProp::Counters { GE, 2 }` requiring the *graveyard* card to already | ||
| //! hold two time counters. Per CR 122.2 a card in a graveyard has no counters, | ||
| //! so that filter is vacuous — the trigger had NO legal target and the counter | ||
| //! *placement* was dropped entirely. The fix (`split_counterless_enter_counters`) | ||
| //! recognizes the counterless origin zone and lifts the clause onto the exile's | ||
| //! `enter_with_counters` (the CR 702.62a suspend template), where the resolver | ||
| //! stamps the counters as the card enters Exile. | ||
| //! | ||
| //! This drives the real pipeline (declare attackers → YouAttack trigger → | ||
| //! ChangeZone resolution) and discriminates the fix: | ||
| //! (a) the graveyard card is exiled (pre-fix it is not even a legal target); | ||
| //! (b) it carries two time counters (CR 702.62a); | ||
| //! (c) Doom's Time Platform itself stays on the battlefield. | ||
|
|
||
| use engine::game::combat::AttackTarget; | ||
| use engine::game::scenario::{GameScenario, P0, P1}; | ||
| use engine::types::actions::GameAction; | ||
| use engine::types::counter::CounterType; | ||
| use engine::types::game_state::WaitingFor; | ||
| use engine::types::zones::Zone; | ||
| use engine::types::Phase; | ||
|
|
||
| const DOOMS_TIME_PLATFORM: &str = "Whenever you attack, exile target nonland card \ | ||
| from your graveyard with two time counters on it. If it doesn't have suspend, \ | ||
| it gains suspend."; | ||
|
|
||
| /// CR 122.2 + CR 702.62a: the graveyard card selected by Doom's Time Platform's | ||
| /// attack trigger must be exiled with two time counters — not filtered out for | ||
| /// lacking counters it can never have in a graveyard. | ||
| #[test] | ||
| fn dooms_time_platform_exiles_graveyard_card_with_two_time_counters() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
|
|
||
| // Doom's Time Platform on P0's battlefield (its type is irrelevant to the | ||
| // "you attack" trigger; the fixture uses a noncreature permanent so it is | ||
| // not itself a candidate attacker). | ||
| let platform = scenario | ||
| .add_enchantment_from_oracle(P0, "Doom's Time Platform", DOOMS_TIME_PLATFORM) | ||
| .id(); | ||
|
|
||
| // A nonland (creature) card in P0's own graveyard — the trigger's target. | ||
| let graveyard_card = scenario | ||
| .add_creature_to_graveyard(P0, "Grizzly Bear", 2, 2) | ||
| .id(); | ||
|
|
||
| // A separate creature to attack with, firing "Whenever you attack". | ||
| let attacker = scenario.add_creature(P0, "Runeclaw Bear", 2, 2).id(); | ||
|
|
||
| let mut runner = scenario.build(); | ||
|
|
||
| runner.advance_to_combat(); | ||
| runner | ||
| .declare_attackers(&[(attacker, AttackTarget::Player(P1))]) | ||
| .expect("declare attacker to fire the you-attack trigger"); | ||
|
|
||
| // Drive the YouAttack trigger: choose the (only) legal graveyard target and | ||
| // let it resolve. `choose_first_legal_target` panics pre-fix because the | ||
| // spurious `Counters GE 2` filter leaves the graveyard card illegal. | ||
| for _ in 0..40 { | ||
| match runner.state().waiting_for.clone() { | ||
| WaitingFor::OrderTriggers { .. } => { | ||
| engine::game::triggers::drain_order_triggers_with_identity(runner.state_mut()); | ||
| } | ||
| WaitingFor::TriggerTargetSelection { .. } | WaitingFor::TargetSelection { .. } => { | ||
| runner.choose_first_legal_target().expect( | ||
| "graveyard card must be a legal target once the counter clause is lifted", | ||
| ); | ||
| } | ||
| WaitingFor::Priority { .. } => { | ||
| if runner.state().stack.is_empty() { | ||
| break; | ||
| } | ||
| if runner.act(GameAction::PassPriority).is_err() { | ||
| break; | ||
| } | ||
| } | ||
| _ => break, | ||
| } | ||
| } | ||
|
|
||
| let card = runner | ||
| .state() | ||
| .objects | ||
| .get(&graveyard_card) | ||
| .expect("graveyard card object must still exist"); | ||
|
|
||
| // (a) The graveyard card was exiled. | ||
| assert_eq!( | ||
| card.zone, | ||
| Zone::Exile, | ||
| "CR 400.7: the targeted graveyard card must move to Exile, got {:?}", | ||
| card.zone, | ||
| ); | ||
| assert!( | ||
| runner.state().exile.contains(&graveyard_card), | ||
| "the exiled card must be in the exile zone", | ||
| ); | ||
|
|
||
| // (b) It carries exactly two time counters (CR 702.62a). Pre-fix this is 0 | ||
| // (the placement was dropped) — and the card would not even be a legal | ||
| // target — so this assertion flips when the fix is reverted. | ||
| let time = card.counters.get(&CounterType::Time).copied().unwrap_or(0); | ||
| assert_eq!( | ||
| time, 2, | ||
| "CR 702.62a: the exiled card must enter with two time counters, got {time}", | ||
| ); | ||
|
|
||
| // (c) Doom's Time Platform itself is untouched — it stays on the battlefield. | ||
| assert!( | ||
| runner.state().battlefield.contains(&platform), | ||
| "Doom's Time Platform must remain on the battlefield, not exile itself", | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Complete the parser regression matrix.
The library test does not assert
origin: Some(Zone::Library)orFilterProp::InZone { Zone::Library }. An origin-loss regression can still pass while applying counters to the wrong target class.Add parser cases for
Zone::Handand a non-counterwith …clause. The helper has explicit behavior for both cases.As per path instructions, add parser tests for graveyard, hand, library, battlefield, and non-counter variants.
🤖 Prompt for AI Agents
Source: Path instructions