ship/fix engine preserve tracked delayed target pins 2 - #7283
ship/fix engine preserve tracked delayed target pins 2#7283matthewevans wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesParent-target incarnation handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DelayedTrigger
participant normalize_contextual_filter
participant ChangeZoneAll
participant TargetObject
DelayedTrigger->>normalize_contextual_filter: Normalize ParentTarget filter
normalize_contextual_filter-->>DelayedTrigger: Return concrete filter
DelayedTrigger->>ChangeZoneAll: Preserve TrackedSetFiltered target
ChangeZoneAll->>TargetObject: Check incarnation pin
TargetObject-->>ChangeZoneAll: Return matching incarnation
ChangeZoneAll->>TargetObject: Move matching object
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ffe10cd to
d98efd0
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
d98efd0 to
984f2eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/delayed_trigger.rs`:
- Around line 989-995: Update change_zone::resolve_all to clean up concrete
tracked-set ids for both TargetFilter::TrackedSet and
TargetFilter::TrackedSetFiltered, including removal from tracked_object_sets and
tracked_set_member_causes. Preserve existing resolution behavior, and add a
regression covering cleanup of both maps for each tracked-set variant.
In `@crates/engine/src/game/filter.rs`:
- Around line 1029-1033: Update the documentation comment above the contextual
parent-target normalization to remove the inaccurate CR 608.2c citation, or
replace it with a verified citation whose rule explicitly requires this
resolution before filter scanning; do not cite CR 608.2c unless the comment
describes resolving written instructions in order.
- Around line 1084-1087: Update the TargetFilter::ParentTargetSlot handling to
return TargetFilter::None when the positive parent target index is absent,
preventing missing slots from matching all targets. Preserve TargetFilter::Any
only for the negated-slot case, and ensure multi-target consumers retain the
declared target-slot index.
- Around line 1100-1109: Update normalize_contextual_filter to remove the
wildcard arm and explicitly handle every leaf variant by cloning it, while
recursively normalizing every nested filter. Include recursion through
ChosenDamageSource.filter and preserve the existing handling of
TrackedSetFiltered, so adding future contextual variants requires deliberate
matching.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85ee9b96-cfc3-4cc3-b72e-2eac3447d90b
📒 Files selected for processing (4)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/filter.rs
cee468c to
434d57c
Compare
|
Follow-up repair pushed after fresh CI exposed three real regressions. The incarnation check now applies only to |
|
Follow-up pushed: Niko uses a single |
1670870 to
661efca
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/change_zone.rs`:
- Around line 1661-1671: Update the tracked-set pin validation and cleanup
around tracked_members_name_parent_object to use the existing recursive filter
helpers, including normalize_contextual_filter and recursive parent-object
reference detection, so nested And, Or, and Not wrappers are traversed. Collect
every concrete tracked-set ID from the normalized filter, apply the pin rule to
matching members, and remove both tracked_object_sets and
tracked_set_member_causes entries for every consumed set. Add regression
coverage for nested And, Or, and Not filters.
- Around line 483-495: Update the Exile-to-Battlefield delayed-return branch
around effective_targets to validate the post-Exile zone-change identity or
incarnation pin before cloning ability.targets, so it does nothing if the object
leaves and re-enters Exile. Annotate this delayed-return identity check with
verified descriptive CR 603.7c language, and add a regression test covering
Exile → another zone → Exile → delayed return.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1b99714-09b3-4567-84a3-6679e95b6a4a
📒 Files selected for processing (4)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/filter.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/effects/delayed_trigger.rs
- crates/engine/src/game/filter.rs
79f1181 to
0180e5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/src/game/engine.rs (1)
16031-16040: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a drift-log entry for the
effects/mod.rs+4 shift.The three
game/effects/mod.rspins moved by a uniform +4. The comment block directly above ends with the#7018,#6958, and#6976adjudications, none of which accounts for this round's +4.The assertion message at Lines 15930-15934 requires each census move to be adjudicated in this document with the site named, not merely re-numbered. The
game/engine.rsentry follows that rule: Lines 16353-16357 record the#7128cause and restate set preservation. Apply the same treatment to these three pins: name the commit or change that inserted the four lines above all three producers, and state that the producers are unchanged and the partition stays 5/7/25.Run the following script to locate the four inserted lines and confirm the producers are unchanged:
#!/bin/bash # Description: Re-derive the three effects/mod.rs CR 603.5 producer coordinates by content. set -euo pipefail f=crates/engine/src/game/effects/mod.rs rg -nP 'WaitingFor::OptionalEffectChoice \{' "$f" | rg -v '\.\.' for n in 6304 6381 9574; do printf '=== %s:%s ===\n' "$f" "$n" sed -n "${n}p" "$f" | sha256sum done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/engine.rs` around lines 16031 - 16040, Add a drift-log entry immediately above the three game/effects/mod.rs pins identifying the commit or change that inserted four lines above all producers. State that the producers remain unchanged and explicitly record preservation of the 5/7/25 partition, while retaining the existing `#7018`, `#6958`, and `#6976` adjudications.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/change_zone.rs`:
- Around line 524-535: Update the special effective_targets branch in the
change-zone resolution logic to also require non-empty
ability.target_incarnations. Keep the delayed_exile_return_targets path gated by
the existing zone, destination, and ParentTarget checks, and use
resolved_targets for unpinned abilities with no target incarnations.
- Around line 1701-1708: Update the tracked-set pin-validation condition near
tracked_members_name_parent_object so ParentTarget-based delayed ChangeZone
effects retain validation when dest_zone is Battlefield; do not disable it
solely because of the destination zone. Add a regression test covering a
Battlefield return and document the specific card-related exemption for ordinary
tracked-set returns, preserving that exemption without weakening parent-bound
validation.
---
Nitpick comments:
In `@crates/engine/src/game/engine.rs`:
- Around line 16031-16040: Add a drift-log entry immediately above the three
game/effects/mod.rs pins identifying the commit or change that inserted four
lines above all producers. State that the producers remain unchanged and
explicitly record preservation of the 5/7/25 partition, while retaining the
existing `#7018`, `#6958`, and `#6976` adjudications.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a34034c-083b-460e-b1fc-a7e9e5b10474
📒 Files selected for processing (3)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/engine.rscrates/engine/src/game/filter.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/filter.rs
15ff09e to
063cddb
Compare
1 participant
Summary by CodeRabbit