Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/engine/src/ai_support/shortcut_efficacy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ fn ability_window_reach(def: &AbilityDefinition) -> WindowReach {
repeat_for,
announced_x,
repeat_until,
optional_player,
optional_for,
iteration_kind_binding,
// ---- read-free ----
Expand Down Expand Up @@ -691,6 +692,7 @@ fn ability_window_reach(def: &AbilityDefinition) -> WindowReach {
|| repeat_for.is_some()
|| announced_x.is_some()
|| repeat_until.is_some()
|| optional_player.is_some()
|| optional_for.is_some()
|| iteration_kind_binding.is_some();
acc.or(WindowReach::of(!conservative_when_present))
Expand Down
8 changes: 8 additions & 0 deletions crates/engine/src/game/ability_rw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3753,6 +3753,7 @@ fn walk_ability(
context: _,
optional_targeting: _,
optional: _,
optional_player,
optional_for: _,
target_choice_timing: _,
description: _,
Expand Down Expand Up @@ -3832,6 +3833,9 @@ fn walk_ability(
if let Some(tc) = target_chooser {
acc.merge(rw_target_filter(tc));
}
if let Some(player) = optional_player {
acc.merge(rw_target_filter(player));
}
if let Some(ru) = repeat_until {
acc.merge(rw_repeat_continuation(ru));
}
Expand Down Expand Up @@ -3886,6 +3890,7 @@ fn walk_definition(
ability_tag: _,
optional_targeting: _,
optional: _,
optional_player,
optional_for: _,
target_choice_timing: _,
distribute: _,
Expand Down Expand Up @@ -3951,6 +3956,9 @@ fn walk_definition(
if let Some(tc) = target_chooser {
acc.merge(rw_target_filter(tc));
}
if let Some(player) = optional_player {
acc.merge(rw_target_filter(player));
}
if let Some(ru) = repeat_until {
acc.merge(rw_repeat_continuation(ru));
}
Expand Down
16 changes: 16 additions & 0 deletions crates/engine/src/game/ability_scan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,9 @@
scoped_player: _, // player id (iteration binding)
kind: _, // AbilityKind tag (no payload)
context: _, // SpellContext: cast-time fact snapshot, not a live read
optional_targeting: _, // bool

Check warning on line 243 in crates/engine/src/game/ability_scan.rs

View workflow job for this annotation

GitHub Actions / Rust lint (fmt, clippy, parser gate)

Diff in /home/runner/work/phase/phase/crates/engine/src/game/ability_scan.rs
optional: _, // bool
optional_player,
optional_for: _, // OpponentMayScope: AnyOpponent/AnyPlayer, no read
target_choice_timing: _, // Stack/Resolution tag
description: _, // display string
Expand Down Expand Up @@ -324,6 +325,13 @@
mode,
));
}
if let Some(player) = optional_player {
acc = acc.or(scan_target_filter(
player,
FilterReadContext::SnapshotOrEvent,
mode,
));
}
// CR 608.2c / CR 107.1c: a "repeat this process while <condition>" predicate is
// re-evaluated against freshly-resolved state each iteration — a resolution read.
if let Some(repeat_until) = repeat_until {
Expand Down Expand Up @@ -4353,6 +4361,7 @@
ability_tag: _,
optional_targeting: _,
optional: _,
optional_player,
optional_for: _,
target_choice_timing: _,
min_x_value: _,
Expand Down Expand Up @@ -4413,6 +4422,13 @@
mode,
));
}
if let Some(player) = optional_player {
acc = acc.or(scan_target_filter(
player,
FilterReadContext::SnapshotOrEvent,
mode,
));
}
if let Some(ru) = repeat_until {
acc = acc.or(scan_repeat_continuation(ru, mode));
}
Expand Down
1 change: 1 addition & 0 deletions crates/engine/src/game/ability_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ pub fn build_resolved_from_def_with_targets(
}
resolved.optional_targeting = def.optional_targeting;
resolved.optional = def.optional;
resolved.optional_player = def.optional_player.clone();
resolved.optional_for = def.optional_for;
resolved.multi_target = def.multi_target.clone();
// CR 115.1 + CR 601.2c: Carry the target-set constraints (e.g. combined
Expand Down
12 changes: 12 additions & 0 deletions crates/engine/src/game/effects/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7123,6 +7123,18 @@
return player;
}
}
// CR 608.2d: a parser-stamped subject such as "they may" names the player
// who receives this choice. The reference resolves from the trigger event,

Check warning on line 7127 in crates/engine/src/game/effects/mod.rs

View workflow job for this annotation

GitHub Actions / Rust lint (fmt, clippy, parser gate)

Diff in /home/runner/work/phase/phase/crates/engine/src/game/effects/mod.rs
// preserving the event-time controller rather than inferring from effect shape.
if let Some(optional_player) = &ability.optional_player {
if let Some(player) = crate::game::targeting::resolve_effect_player_ref(
state,
ability,
optional_player,
) {
return player;
}
}
if let Effect::Sacrifice { target, .. } = &ability.effect {
if target_filter_controller_scope(target) == Some(ControllerRef::ParentTargetController) {
if let Some(player) = crate::game::targeting::resolve_effect_player_ref(
Expand Down
1 change: 1 addition & 0 deletions crates/engine/src/game/resolution_prompt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ pub(crate) fn chain_offers_choice(a: &ResolvedAbility) -> bool {
sub_ability,
else_ability,
optional,
optional_player: _, // selects the optional actor; `optional` already records the choice
optional_for,
optional_targeting,
unless_pay,
Expand Down
9 changes: 9 additions & 0 deletions crates/engine/src/game/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3084,6 +3084,7 @@ fn self_counter_ability_is_batch_candidate(ability: &ResolvedAbility) -> bool {
context,
optional_targeting,
optional,
optional_player,
optional_for,
multi_target,
target_constraints,
Expand Down Expand Up @@ -3144,6 +3145,7 @@ fn self_counter_ability_is_batch_candidate(ability: &ResolvedAbility) -> bool {
&& *context == SpellContext::default()
&& !*optional_targeting
&& !*optional
&& optional_player.is_none()
&& optional_for.is_none()
&& multi_target.is_none()
&& target_constraints.is_empty()
Expand Down Expand Up @@ -3297,6 +3299,7 @@ fn fixed_controller_gain_life_ability_is_batch_candidate(ability: &ResolvedAbili
context,
optional_targeting,
optional,
optional_player,
optional_for,
multi_target,
target_constraints,
Expand Down Expand Up @@ -3352,6 +3355,7 @@ fn fixed_controller_gain_life_ability_is_batch_candidate(ability: &ResolvedAbili
&& *context == SpellContext::default()
&& !*optional_targeting
&& !*optional
&& optional_player.is_none()
&& optional_for.is_none()
&& multi_target.is_none()
&& target_constraints.is_empty()
Expand Down Expand Up @@ -3490,6 +3494,7 @@ fn fixed_opponent_lose_life_ability_is_batch_candidate(ability: &ResolvedAbility
context,
optional_targeting,
optional,
optional_player,
optional_for,
multi_target,
target_constraints,
Expand Down Expand Up @@ -3545,6 +3550,7 @@ fn fixed_opponent_lose_life_ability_is_batch_candidate(ability: &ResolvedAbility
&& *context == SpellContext::default()
&& !*optional_targeting
&& !*optional
&& optional_player.is_none()
&& optional_for.is_none()
&& multi_target.is_none()
&& target_constraints.is_empty()
Expand Down Expand Up @@ -4130,6 +4136,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance(
context: a_context,
optional_targeting: a_optional_targeting,
optional: a_optional,
optional_player: a_optional_player,
optional_for: a_optional_for,
multi_target: a_multi_target,
target_constraints: a_target_constraints,
Expand Down Expand Up @@ -4186,6 +4193,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance(
context: b_context,
optional_targeting: b_optional_targeting,
optional: b_optional,
optional_player: b_optional_player,
optional_for: b_optional_for,
multi_target: b_multi_target,
target_constraints: b_target_constraints,
Expand Down Expand Up @@ -4254,6 +4262,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance(
&& a_context == b_context
&& a_optional_targeting == b_optional_targeting
&& a_optional == b_optional
&& a_optional_player == b_optional_player
&& a_optional_for == b_optional_for
&& a_multi_target == b_multi_target
&& a_target_constraints == b_target_constraints
Expand Down
3 changes: 3 additions & 0 deletions crates/engine/src/parser/oracle_ir/trigger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,9 @@ pub(crate) struct TriggerModifiers {
/// CR 603.5: Some triggered abilities' effects are optional (they contain
/// "may"). They go on the stack regardless; the choice is made on resolution.
pub(crate) optional: bool,
/// CR 608.2d: Event-relative player explicitly named by the root optional
/// subject, after any intervening-if wrapper has been removed.
pub(crate) optional_player: Option<TargetFilter>,
/// CR 118.12: "unless [player] pays {cost}" tax modifier.
pub(crate) unless_pay: Option<UnlessPayModifier>,
/// Intervening-if condition extracted from effect text.
Expand Down
50 changes: 42 additions & 8 deletions crates/engine/src/parser/oracle_trigger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,14 @@
AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, AbilityTag,
AdditionalCostOrigin, AdditionalCostPaymentSource, AggregateFunction, AttachmentKind,
AttackersDeclaredCountSubject, CastManaObjectScope, CastManaSpentMetric, CastVariantPaid,
CoinFlipResult, Comparator, ControllerRef, CountScope, CounterTriggerFilter, DamageKindFilter,

Check warning on line 53 in crates/engine/src/parser/oracle_trigger.rs

View workflow job for this annotation

GitHub Actions / Rust lint (fmt, clippy, parser gate)

Diff in /home/runner/work/phase/phase/crates/engine/src/parser/oracle_trigger.rs
DestinationConstraint, DieResultFilter, Effect, FilterProp, ManaAbilityProducedFilter,
ObjectScope, OriginConstraint, ParsedCondition, PlayerFilter, PlayerScope, PtStat,
DestinationConstraint, DieResultFilter, Effect, EffectScope, FilterProp,
ManaAbilityProducedFilter, ObjectScope, OriginConstraint, ParsedCondition, PlayerFilter,
PlayerScope, PtStat,
PtValueScope, QuantityExpr, QuantityRef, RenownSubject, SacrificeAggregateStat, SacrificeCost,
SacrificeRequirement, SharedQuality, StaticCondition, SubAbilityLink, TapCreaturesRequirement,
TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, TypeFilter, TypedFilter,
UnlessPayModifier, ZoneChangeClause,
TapStateChange, TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition,
TypeFilter, TypedFilter, UnlessPayModifier, ZoneChangeClause,
};
use crate::types::card_type::{is_land_subtype, CoreType};
use crate::types::counter::CounterType;
Expand Down Expand Up @@ -296,6 +297,18 @@
.is_ok()
}

/// CR 608.2d + CR 603.2: A leading "they may" in a normalized trigger body
/// names the player recorded by that trigger event, rather than the ability's
/// controller. Call after stripping an intervening-if wrapper so the actor is
/// retained for both direct and conditional root modals.
fn optional_player_from_effect_body(effect_text: &str) -> Option<TargetFilter> {
let lower = effect_text.to_lowercase();
tag::<_, _, OracleError<'_>>("they may ")
.parse(lower.trim_start())

Check failure on line 307 in crates/engine/src/parser/oracle_trigger.rs

View workflow job for this annotation

GitHub Actions / WASM compile check

`lower` does not live long enough

Check failure on line 307 in crates/engine/src/parser/oracle_trigger.rs

View workflow job for this annotation

GitHub Actions / Card data (generate, validate, coverage)

`lower` does not live long enough
.ok()
.map(|_| TargetFilter::TriggeringPlayer)
}

/// CR 113.6 + CR 113.6b: Collect every zone the trigger's
/// source must occupy for the condition to be satisfiable. Returns the
/// deduplicated union of `SourceInZone { zone }` references across
Expand Down Expand Up @@ -1376,6 +1389,7 @@
(without_if, cond, None)
}
};
let optional_player = optional_player_from_effect_body(&effect_without_if);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// CR 608.2c (resolution-order instructions): "You may" at the start of
// the effect text makes the triggered effect optional at resolution.
Expand Down Expand Up @@ -1606,6 +1620,7 @@
body,
modifiers: TriggerModifiers {
optional,
optional_player,
unless_pay,
intervening_if: if_condition,
trigger_subject,
Expand Down Expand Up @@ -1805,6 +1820,11 @@
// quantities to `PlayerScope::ScopedPlayer` so they resolve against the
// damaged/attacked player rather than an absent chosen target.
let mut execute = execute;
if let Some(optional_player) = &modifiers.optional_player {
if let Some(ability) = execute.as_deref_mut() {
ability.optional_player = Some(optional_player.clone());
}
}
// CR 603.2c: A `TrackedSetAggregate { source: TriggeringBatch }` reduces the
// objects of THIS trigger's event, read back through
// `extract_sources_from_event`. That only yields anything for the events that
Expand Down Expand Up @@ -2123,7 +2143,7 @@
}
}

// CR 608.2k + CR 603.7c: For event-source-bearing trigger modes, the "that
// CR 603.2 + CR 603.6 + CR 608.2k: For event-source-bearing trigger modes, the "that
// card / that creature / that permanent" anaphor in the effect body
// refers to the *triggering object* carried by the event (the just-
// discarded card, sacrificed permanent, drawn card, etc.) — not a chosen
Expand Down Expand Up @@ -2225,7 +2245,7 @@
/// TargetFilter` and whose runtime semantics make sense against the event
/// object (e.g. `ChangeZone` operating on the just-discarded card). Other
/// effect variants are left untouched.
fn lift_parent_target_to_triggering_source(effect: &mut Effect) {
fn lift_parent_target_to_triggering_source(effect: &mut Effect, allow_set_tap_lift: bool) {
// CR 608.2k: each variant carries a top-level `target` that, when the
// surface anaphor was "that <object>", refers to the event object.
let target = match effect {
Expand All @@ -2234,6 +2254,15 @@
// "create a token that's a copy of that creature" (Necroduality) — the
// copy source is the entering object, not the trigger's own source.
Effect::CopyTokenOf { target, .. } => target,
// CR 608.2k + CR 701.26a: on a single-object zone-change trigger,
// "they may tap that permanent" refers to the entering object. The
// caller limits this to the trigger's top-level, untargeted Tap effect;
// a reflexive or targeted tap has its own chosen referent instead.
Effect::SetTapState {
target,
scope: EffectScope::Single,
state: TapStateChange::Tap,
} if allow_set_tap_lift => target,
_ => return,
};
if matches!(target, TargetFilter::ParentTarget) {
Expand All @@ -2260,7 +2289,8 @@
None
}

/// CR 608.2k + CR 603.7c: Recurse `lift_parent_target_to_triggering_source`
/// CR 603.2 + CR 603.6 + CR 608.2k: Recurse
/// `lift_parent_target_to_triggering_source`
/// through an ability's effect AND every chained `sub_ability`. Required
/// for the punisher-trigger class: a chained Tergrid-shape ability like
/// "...exile that card, then create a token" carries the "that card"
Expand All @@ -2276,6 +2306,7 @@
// Necroduality (top-level `CopyTokenOf` with no prior choice) and Tergrid
// ("put that card …, then create a token") still lift correctly.
let mut node = Some(ability);
let mut is_top_level = true;
while let Some(link) = node {
// CR 701.23a: A library search's continuation receives the found cards
// as its parent targets. In an event-source-bearing trigger, the
Expand All @@ -2294,8 +2325,11 @@
if introduces_chosen_object_target(link.effect.as_ref()) {
break;
}
lift_parent_target_to_triggering_source(link.effect.as_mut());
let allow_set_tap_lift =
is_top_level && link.multi_target.is_none() && !link.optional_targeting;
lift_parent_target_to_triggering_source(link.effect.as_mut(), allow_set_tap_lift);
node = link.sub_ability.as_deref_mut();
is_top_level = false;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

Expand Down
Loading
Loading