diff --git a/crates/mtgish-import/src/convert/action.rs b/crates/mtgish-import/src/convert/action.rs index dcebaddee3..323f1d54c3 100644 --- a/crates/mtgish-import/src/convert/action.rs +++ b/crates/mtgish-import/src/convert/action.rs @@ -245,11 +245,20 @@ impl VariableBindings { /// typed constraint from the outer `Actions::Targeted` wrapper. This /// preserves the typed target slot so the engine can prompt for targets /// at cast/activation time (CR 601.2c). + /// + /// Selection continuations (`is_selection_continuation`) are skipped: their + /// `TargetFilter::Any` is bound by the preceding effect's choice, not a + /// target slot the outer wrapper may constrain. fn rewrite_target_filters(&self, effects: &mut [Effect]) { let Some(typed) = &self.target_filter else { return; }; - for effect in effects.iter_mut() { + for idx in 0..effects.len() { + let (preceding, from_here) = effects.split_at_mut(idx); + let effect = &mut from_here[0]; + if is_selection_continuation(preceding.last(), effect) { + continue; + } rewrite_any_target_filter_in_effect(effect, typed); } } @@ -5644,13 +5653,7 @@ fn apply_player_target_chain( ) -> ConvResult> { let mut out = Vec::with_capacity(effects.len()); for effect in effects { - let hand_selection_continuation = matches!(out.last(), Some(Effect::RevealHand { .. })) - && is_selected_hand_exile_continuation(&effect); - let library_selection_continuation = - matches!(out.last(), Some(Effect::SearchLibrary { .. })) - && is_search_library_change_zone_continuation(&effect); - - if hand_selection_continuation || library_selection_continuation { + if is_selection_continuation(out.last(), &effect) { out.push(effect); } else { out.push(apply_player_target(effect, target_filter.clone())?); @@ -5659,6 +5662,27 @@ fn apply_player_target_chain( Ok(out) } +/// CR 115.1 + CR 601.2c: Does `effect` consume a selection already bound by the +/// immediately preceding effect — the card found by `Effect::SearchLibrary`, the +/// card chosen by `Effect::RevealHand` — rather than declare a target slot of +/// its own? +/// +/// Such an effect's `TargetFilter::Any` is structural: the engine resolves it +/// from the continuation the previous effect installed, so no player or typed +/// constraint from an enclosing wrapper may overwrite it. Both rebinding passes +/// (`apply_player_target_chain` for the `SearchPlayersLibrary` player axis and +/// `VariableBindings::rewrite_target_filters` for the outer `Actions::Targeted` +/// typed axis) route through this single predicate — an outer wrapper that +/// clobbered the slot would retarget the move at an arbitrary permanent the +/// bound player controls instead of the card the search selected. +fn is_selection_continuation(preceding: Option<&Effect>, effect: &Effect) -> bool { + match preceding { + Some(Effect::RevealHand { .. }) => is_selected_hand_exile_continuation(effect), + Some(Effect::SearchLibrary { .. }) => is_search_library_change_zone_continuation(effect), + _ => false, + } +} + fn is_selected_hand_exile_continuation(effect: &Effect) -> bool { matches!( effect, diff --git a/crates/mtgish-import/src/convert/replacement.rs b/crates/mtgish-import/src/convert/replacement.rs index a11778a778..88eca1baf6 100644 --- a/crates/mtgish-import/src/convert/replacement.rs +++ b/crates/mtgish-import/src/convert/replacement.rs @@ -339,6 +339,16 @@ fn event_to_damage_filters( }, // Combat-only / noncombat-only restrictors set `combat_scope` per // CR 614.1a. + // + // CR 510.1a: The unqualified "combat damage would be dealt" event names + // neither a source nor a recipient, so both filter slots stay `None` + // and only the combat scope narrows the replacement. Mirrors the same + // variant's handling in `damage_event_to_prevent_params`. + E::CombatDamageWouldBeDealt => DamageEventFilters { + source_filter: None, + target_filter: None, + combat_scope: Some(CombatDamageScope::CombatOnly), + }, E::CombatDamageWouldBeDealtByACreatureToRecipient(_perm, recipient) => DamageEventFilters { source_filter: None, target_filter: recipient_to_damage_target_filter(recipient), @@ -3287,8 +3297,8 @@ fn expiration_tag(e: &Expiration) -> String { #[cfg(test)] mod tests { use engine::types::ability::{ - AbilityCost, ContinuousModification, DamageModification, Duration, Effect, QuantityExpr, - ReplacementMode, TargetFilter, + AbilityCost, CombatDamageScope, ContinuousModification, DamageModification, Duration, + Effect, QuantityExpr, ReplacementMode, TargetFilter, }; use engine::types::card_type::{CoreType, Supertype}; use engine::types::keywords::Keyword; @@ -3317,6 +3327,30 @@ mod tests { ) .expect("fixed damage replacement actions should convert"); + assert_eq!(defs.len(), 3); + + // CR 510.1a: the unqualified event names neither a damage source nor a + // recipient, so both filter slots must stay unset and the combat scope + // is the ONLY narrowing the event contributes. Asserted on every + // definition the action list produced, since they all share one event. + // Without this, a wrong scope or an over-narrow filter would still + // leave the `damage_modification` assertions below green. + for (idx, def) in defs.iter().enumerate() { + assert_eq!( + def.damage_source_filter, None, + "defs[{idx}]: unqualified combat event must not narrow the damage source" + ); + assert_eq!( + def.damage_target_filter, None, + "defs[{idx}]: unqualified combat event must not narrow the damage target" + ); + assert_eq!( + def.combat_scope, + Some(CombatDamageScope::CombatOnly), + "defs[{idx}]: combat-damage event must restrict the replacement to combat damage" + ); + } + assert!(matches!( defs[0].damage_modification.as_ref(), Some(DamageModification::PreventionMinus { value: u32::MAX }) diff --git a/crates/mtgish-import/src/diff/ordering.rs b/crates/mtgish-import/src/diff/ordering.rs index 02ca2a336c..d55f15254f 100644 --- a/crates/mtgish-import/src/diff/ordering.rs +++ b/crates/mtgish-import/src/diff/ordering.rs @@ -376,6 +376,221 @@ pub const ORDERING_MANIFEST: &[((&str, &str), OrderingClass)] = &[ ), // ----- Trigger cause filters ----- (("TriggerCause", "core_types"), OrderingClass::SetEquivalent), + // ----- AbilityBlockReason ----- + // CR 602.5: the field's own contract is "sorted, deduped permanents" — a + // canonicalized set of independently-prohibiting sources. Set. + ( + ("AbilityBlockReason", "sources"), + OrderingClass::SetEquivalent, + ), + // ----- AbilityCondition (RevealedHasCardType) ----- + // CR 205.1a: disjunctive core-type match against the revealed card — only + // membership decides the outcome. Matches `QuantityRef::card_types`. Set. + ( + ("AbilityCondition", "card_types"), + OrderingClass::SetEquivalent, + ), + // ----- AbilityDefinitionDe (serde mirror of AbilityDefinition) ----- + // The deserialization mirror must classify identically to the type it + // reconstructs, or the same list would diff differently depending on which + // shape the JSON deserialized through. + ( + ("AbilityDefinitionDe", "activation_restrictions"), + OrderingClass::SetEquivalent, + ), + ( + ("AbilityDefinitionDe", "mode_abilities"), + OrderingClass::OrderSignificant, + ), + ( + ("AbilityDefinitionDe", "target_constraints"), + OrderingClass::SetEquivalent, + ), + // ----- CardFace / CardMetadata catalog lists ----- + // CR 717.1: the set of lit-up roll numbers on an Attraction variant. Which + // numbers are lit decides the roll; their listing order does not. Set. + ( + ("CardFace", "attraction_lights"), + OrderingClass::SetEquivalent, + ), + // Alchemy spellbook — the fixed candidate pool a draft effect selects from. + // Membership defines the pool; MTGJSON's listing order is incidental. Set. + (("CardMetadata", "spellbook"), OrderingClass::SetEquivalent), + // ----- CastingPermission ----- + // CR 613.1d: enters-with continuous modifications carried on a cast grant. + // Layer assignment is by modification variant, so the layer system re-sorts + // them at apply time. Mirrors `StaticDefinition::modifications`. Set. + ( + ("CastingPermission", "enters_with_modifications"), + OrderingClass::SetEquivalent, + ), + // ----- Effect embedded lists (continued) ----- + // CR 122.1: entry-time counters gated per-filter. Counter placement is + // cumulative and filter-keyed, so reordering the riders cannot change the + // resulting counter set. Mirrors `Effect::enter_with_counters`. Set. + ( + ("Effect", "conditional_enter_with_counters"), + OrderingClass::SetEquivalent, + ), + // CR 707.9: "except …" modifications applied to a created copy. Same + // layer-resorted rationale as `StaticDefinition::modifications`. Set. + ( + ("Effect", "copy_modifications"), + OrderingClass::SetEquivalent, + ), + (("Effect", "modifications"), OrderingClass::SetEquivalent), + // Zone union searched for candidate cards — membership, not order. Set. + (("Effect", "zones"), OrderingClass::SetEquivalent), + // ----- FaceDownProfile ----- + // CR 708.2a + CR 205.1a: the core types and subtypes a face-down permanent + // is given. Type/subtype sets are unordered. Set. + ( + ("FaceDownProfile", "extra_core_types"), + OrderingClass::SetEquivalent, + ), + ( + ("FaceDownProfile", "subtypes"), + OrderingClass::SetEquivalent, + ), + // ----- ManaSpendRestriction ----- + // CR 106.6: the field's own contract is a DISJUNCTION of cost criteria — + // any one match satisfies it, so order cannot matter. Set. + ( + ("ManaSpendRestriction", "criteria"), + OrderingClass::SetEquivalent, + ), + // ----- ModalChoice ----- + // CR 700.2i: per-mode pawprint weights are index-parallel with the modes + // themselves — `mode_pawprints[i]` is the cost of mode `i`. Reordering + // reprices every mode. Positional. + ( + ("ModalChoice", "mode_pawprints"), + OrderingClass::OrderSignificant, + ), + // ----- NotedManaPayment ----- + // CR 106.6: the multiset of mana types spent on one activation. Consumers + // ask "was type T among them", never "which came first". Set. + (("NotedManaPayment", "types"), OrderingClass::SetEquivalent), + // ----- PerpetualModification ----- + // CR 205.1a + CR 113.2c: granted keyword and creature-subtype sets. Each + // keyword instance functions independently (CR 113.2c). Set. + ( + ("PerpetualModification", "creature_subtypes"), + OrderingClass::SetEquivalent, + ), + ( + ("PerpetualModification", "keywords"), + OrderingClass::SetEquivalent, + ), + // ----- QuantityRef ----- + // CR 305.2a: origin zones narrowing a land-play count — a membership test + // over the play's recorded origin. Set. + (("QuantityRef", "from_zones"), OrderingClass::SetEquivalent), + // ----- ReplacementCondition ----- + // CR 111.1: core types the proposed token must overlap. Overlap is + // symmetric in the listing order. Set. + ( + ("ReplacementCondition", "core_types"), + OrderingClass::SetEquivalent, + ), + // ----- During-resolution cast state (Cascade / Discover / Ripple) ----- + // CR 608.2g: dig cards that were not the hit. CR 702.60a bottoms the + // non-cast reveals "in any order", so their recorded order is not + // rules-meaningful. Mirrors `CastPermissionConstraint::exiled_misses`. Set. + ( + ("ResolutionCastCleanup", "exiled_misses"), + OrderingClass::SetEquivalent, + ), + // CR 607.2a: the "exiled this way" batch a re-offer's candidate set is + // confined to — a membership restriction. Set. + ( + ("ResolutionCastSuccessAction", "member_pool"), + OrderingClass::SetEquivalent, + ), + // CR 702.60a: Ripple may cast ANY NUMBER of the same-named reveals, so the + // remaining pool is a candidate set, not a queue with meaningful order. Set. + ( + ("ResolutionCastSuccessAction", "remaining_hits"), + OrderingClass::SetEquivalent, + ), + // Zone union searched for free-cast candidates. Matches `Effect::zones`. Set. + ( + ("ResolutionCastSuccessAction", "zones"), + OrderingClass::SetEquivalent, + ), + // ----- ResolvedAbility (continued) ----- + // Ids stripped from this ability's own candidate lists — read as a + // membership test, never as a positional referent. Set. + ( + ("ResolvedAbility", "cost_paid_object_ids"), + OrderingClass::SetEquivalent, + ), + // CR 700.2b: one definition per mode; mode order is the player-facing + // label order. Mirrors `AbilityDefinition::mode_abilities`. Positional. + ( + ("ResolvedAbility", "mode_abilities"), + OrderingClass::OrderSignificant, + ), + // CR 700.2d: the field's own contract is "in the printed instruction order + // used to resolve them" — order IS the resolution sequence. Positional. + ( + ("ResolvedAbility", "selected_mode_labels"), + OrderingClass::OrderSignificant, + ), + // CR 115.1 + CR 601.2c: independent legality predicates ANDed together. + // Mirrors `AbilityDefinition::target_constraints`. Set. + ( + ("ResolvedAbility", "target_constraints"), + OrderingClass::SetEquivalent, + ), + // CR 400.7 + CR 603.7c: incarnation pins for the referents in `targets`, + // which is itself positional — pin `i` guards target `i`. Reordering + // re-binds each pin to a different target. Positional. + ( + ("ResolvedAbility", "target_incarnations"), + OrderingClass::OrderSignificant, + ), + // ----- SpellContext (continued) ----- + // CR 113.2c + CR 601.2b: the non-kicker analogue of `kickers_paid`, read + // the same way (was cost C paid, how many times). Set. + ( + ("SpellContext", "additional_cost_payments"), + OrderingClass::SetEquivalent, + ), + // CR 700.2d: mode indices are stored ASCENDING with repeats — the ordering + // is a normalization, and the multiset of chosen modes is the meaning. + // (Player-facing resolution order lives in `selected_mode_labels`.) Set. + ( + ("SpellContext", "chosen_modes"), + OrderingClass::SetEquivalent, + ), + // ----- StaticMode (continued) ----- + // CR 702.122a: which crew-like keyword actions a power/toughness + // contribution modifier applies to. Membership. Set. + (("StaticMode", "actions"), OrderingClass::SetEquivalent), + // CR 122.2: destination zones excluded from counter persistence — the + // "any zone other than [zones]" set. Set. + ( + ("StaticMode", "excluded_zones"), + OrderingClass::SetEquivalent, + ), + // ----- TriggerCause / TriggerDefinition ----- + // CR 603.6a: the field's own contract marks these qualifiers disjunctive. Set. + (("TriggerCause", "qualifiers"), OrderingClass::SetEquivalent), + // CR 106.1: the produced-mana set a "taps for {C}" trigger requires at + // least one match in — an overlap test. Set. + ( + ("TriggerDefinition", "taps_for_mana_produced"), + OrderingClass::SetEquivalent, + ), + // ----- TriggerOccurrenceState ----- + // Grant instances are addressed exclusively by `producer` key (duplicate + // producers are rejected outright) and by `instance` id — never by index. + // A keyed set. Set. + ( + ("TriggerOccurrenceState", "active_grants"), + OrderingClass::SetEquivalent, + ), ]; /// Look up the ordering class for a `(carrier, field)` pair. diff --git a/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json b/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json index b459b3a084..0a26eb0d28 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json @@ -24,7 +24,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -63,7 +62,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json b/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json index e75ceb8d14..11b8db5202 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json @@ -19,7 +19,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -76,7 +75,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json b/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json index befd644010..078fb9463c 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json @@ -26,7 +26,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json b/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json index ba4c386200..e7465927b8 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json @@ -10,7 +10,7 @@ "execute": { "kind": "Spell", "effect": { - "type": "AddCounter", + "type": "PutCounter", "counter_type": "P1P1", "count": { "type": "Ref", @@ -27,7 +27,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json b/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json index ac8c832bf7..6a1b2c91c5 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json @@ -12,7 +12,10 @@ "effect": { "type": "RemoveCounter", "counter_type": "M1M1", - "count": 1, + "count": { + "type": "Fixed", + "value": 1 + }, "target": { "type": "SelfRef" } @@ -22,7 +25,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -50,7 +52,10 @@ "effect": { "type": "RemoveCounter", "counter_type": "M1M1", - "count": 1, + "count": { + "type": "Fixed", + "value": 1 + }, "target": { "type": "SelfRef" } @@ -60,7 +65,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -89,7 +93,7 @@ "execute": { "kind": "Spell", "effect": { - "type": "AddCounter", + "type": "PutCounter", "counter_type": "M1M1", "count": { "type": "Fixed", @@ -104,7 +108,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json b/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json index cd0a74aea5..7c3a00a2bb 100644 --- a/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json +++ b/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json @@ -24,7 +24,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false,