fix(engine): rebind DamageAll recipient-filter source to the resolved damage target#5873
Conversation
… damage target (phase-rs#4759) Issue phase-rs#4759 "bug 1" reported Chandra's Ignition ("Target creature you control deals damage equal to its power to each other creature and each opponent") prompting for a second target when the controller had multiple creatures. That specific symptom does NOT reproduce on current main: the generalized one-sided-damage target-slot suppression (added for phase-rs#4234 Bite Down, extended to DamageAll's damage_source: Target shape by the maintainer's own "phase-rs#607 followup" commit) already collapses the recipient's "each other creature"/"each opponent" clause into a non-targeted DamageAll sub-ability, so casting only ever prompts for the one real target (the source creature). While building the end-to-end regression test the dispatch asked for (cast with >=2 own creatures + an opponent creature, drive the real pipeline), a distinct, real bug surfaced: the chosen source creature damaged ITSELF. DamageAll::resolve_all built its recipient-matching FilterContext via FilterContext::from_ability(ability), which sets source_id to the ability's nominal source (the spell object). FilterProp::Another ("each OTHER creature") compares each battlefield candidate's id against that source_id — but the spell is never itself a battlefield creature, so the comparison never excludes anything, and the resolved damage-source creature (bound separately, only for keyword/protection purposes, via damage_source: Some(DamageSource::Target)) was never excluded from its own "each other creature" recipient set. Fix: when damage_source is Some(DamageSource::Target), rebind the recipient FilterContext's source_id/source_controller to the resolved target creature before evaluating the recipient filter, mirroring the existing damage-source lookup used later in the same function for keyword/replacement purposes. Scoped to the DamageSource::Target case only (the shape Chandra's Ignition and Nova Flame use); other DamageAll damage-source modes are untouched. Added issue_4759_chandras_ignition_single_target.rs: drives the real cast/ target-selection pipeline with the controller's chosen source creature, a second creature the controller also controls, and an opponent creature. Asserts exactly one target prompt occurs (pinning bug 1's already-fixed behavior) and that only the two OTHER creatures + the opponent player take damage, never the source itself (pinning the newly-fixed self-damage bug). Failed pre-fix (source took damage_marked == 4 instead of 0); passes after. Full suite green: fmt clean, clippy -D warnings clean, lib 16673 passed/0 failed, integration 3133 passed/0 failed. Closes phase-rs#4759
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves an issue where "other"-relative recipient filters (such as Chandra's Ignition) were incorrectly evaluated against the ability's nominal source instead of the resolved damage source creature. This is fixed by updating the filter context's source ID and controller when the damage source is a target. An integration test has been added to verify this behavior. The reviewer suggests refactoring the target resolution logic in deal_damage.rs to avoid duplicate iterations over ability.targets.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if matches!(damage_source, Some(DamageSource::Target)) { | ||
| if let Some(resolved_source_id) = ability.targets.iter().find_map(|t| match t { | ||
| TargetRef::Object(id) => Some(*id), | ||
| _ => None, | ||
| }) { | ||
| ctx.source_id = resolved_source_id; | ||
| ctx.source_controller = state.objects.get(&resolved_source_id).map(|o| o.controller); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic correctly fixes the issue. However, it introduces a small code duplication. The logic to find the resolved_source_id by iterating over ability.targets is repeated later in this function (around line 1597) to create the DamageContext.
To improve maintainability and avoid iterating over ability.targets twice, consider resolving the source ID once at the beginning of the function when damage_source is Some(DamageSource::Target), storing it in a variable, and then using it in both places.
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head implementation review.
2 participants
Fixes #4759.
Chandra's Ignition: "Target creature you control deals damage equal to its power to each other creature and each opponent."
Root cause & fix
The reported "prompts for two targets" symptom is already fixed on
mainvia an earlier, unrelated maintainer commit that added a generalized single-target suppression forEffect::DamageAllwithdamage_source: Some(Target).While building the required end-to-end regression test, found a real, separate bug:
DamageAll::resolve_allbuilt its recipient-matchingFilterContextfromability.source_id(the spell object) instead of the resolvedDamageSource::Targetcreature, soFilterProp::Another("each OTHER creature") never actually excluded the damage source itself — the targeted/boosted creature took its own sweep damage.Fixed by rebinding
ctx.source_id/source_controllerto the resolved target creature before the recipient filter runs, scoped strictly to theDamageSource::Targetcase (mirrors the equivalent fix already applied to the single-recipientDealDamagepath in #5834/issue #4960).Testing
New end-to-end test: cast Chandra's Ignition with multiple creatures on the battlefield, confirm only one target prompt occurs, and confirm the source creature takes zero damage from its own sweep while other creatures and opponents take the full amount.
Verified locally (rebased onto main): fmt clean, clippy clean, 16673 lib tests pass, 3133 integration tests pass.