fix(engine): skip the legality clone for self-sacrifice mana costs - #7558
Merged
Conversation
The mana-availability display sweep cloned the whole GameState once per
mana source whose cost the cheap gate could not conclusively decide.
Treasure's "{T}, Sacrifice this: Add one mana of any color" is exactly
that shape: AbilityCost::Sacrifice is unconditionally uncovered by
all_components_cheap_gate_covered, so every Treasure fell through to
can_activate_mana_ability_by_simulation and a full state clone.
On a real 4-player Commander board (676 objects, 193 Treasure tokens)
derive_display_state took 2233 ms per resolution, with layers_full
growing exactly as N(N+1)/2 -- O(N^2 x battlefield), because each
resolution adds a mana source that the next sweep re-clones.
Widen the skip to whole-tree choice-free costs that sacrifice exactly
the ability's own source, reusing the existing classifier
mana_sources::has_unambiguous_self_sacrifice_component. Aggregate
requirements, Count{n>1} and every non-SelfRef target are excluded by
construction by that predicate's SelfRef / Count{1} match -- a non-self
sacrifice may have no legal victim, so its simulation stays load-bearing.
Two conservative guards keep the answer identical:
* CR 601.2g -- a source already committed to a pending spell's deferred
additional sacrifice cost is reserved, and paying this ability's cost
would then error. Reuses the payment path's own authority,
cost_sacrifices_reserved_source.
* CR 118.3 + CR 601.2h -- the readiness gate evaluates
player_cant_sacrifice_as_cost on the pre-payment state, but payment
re-evaluates it after this tree's {T} component has already tapped the
source, and a prohibition's object filter can read that tapped bit.
An O(1) static-presence read declines the fast path whenever any
CantPayCost static is in play.
Both guards decline into today's exact simulation, so a spurious guard
costs performance and never correctness.
CR 616.1 needs no guard: a replacement on the sacrifice's battlefield to
graveyard move makes sacrifice_permanent return NeedsReplacementChoice,
which the self-sacrifice payment arm maps to Ok(Paused), so the
simulation returns Ok and reports the same answer the fast path does.
The self-sacrifice cheap-gate widening admitted cost trees carrying more
than one consuming leaf. has_unambiguous_self_sacrifice_component was
whole-tree cost_component_choice_free AND cost_has_component, and
cost_has_component requires only that at least one self-sacrifice leaf be
present -- it imposes no arity bound. Because the payer flattens the tree
and pays one leaf at a time, a second consuming leaf re-enters its arm
after the first has already mutated the source:
Composite[Sacrifice(SelfRef,1), Sacrifice(SelfRef,1)]
-> the second leaf hits sacrifice_permanent's not-on-battlefield Err
Composite[Tap, Tap, Sacrifice(SelfRef,1)]
-> the second Tap hits tap_source's already-tapped Err
In both, the simulation answers false while the fast path answered true --
a change of ANSWER, not merely of cost, in the unsafe direction.
Bound the classifier to a flat tree carrying exactly one
Sacrifice(SelfRef, Count{1}) leaf and at most one Tap leaf. The census
recurses so it counts the same multiset append_mana_ability_cost_components
builds; a census modelled on cost_has_component's one-level shape would
report 1 where the payer pays 2.
The flatness clause is required for soundness, not tidiness. A flattened
census alone WIDENS the classifier onto nested
Composite[Composite[Tap, Sacrifice(SelfRef,1)]], which today is rejected
only by cost_has_component's one-level blindness. That same blindness sits
upstream in the readiness gate, which keys its tapped and summoning-sick
checks on the one-level has_tap_component -- so a tapped source with a
nested tree clears readiness, and removing the blindness on the classifier
side alone would let it fast-path to true while the payer errors on the
flattened {T}. Requiring flatness keeps the acceptance set a strict subset
of the previous one, so every consumer can only move toward pre-fix
behavior.
SacrificeRequirement::Aggregate and Count{n>1} remain excluded by
construction through the literal Count{count:1} pattern; no guard was added
for them.
Also document that the CantPayCost guard keys on board-global static
presence rather than on the source, so a single such permanent returns
every mana source on the board to the simulation path.
matthewevans
enabled auto-merge
August 20, 2026 01:01
Contributor
|
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 (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe PR adds a guarded fast path for eligible self-sacrifice mana costs. It strengthens cost classification with flattened leaf counts and adds unit and integration tests for clone elimination and conservative simulation fallback. ChangesSelf-sacrifice legality optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change narrows legality simulation for a bounded class of self-sacrifice mana costs while retaining the existing simulation path for guarded cases; the supplied tests and checks pass, and no actionable merge-blocking risk remains beyond normal review. Sequence Diagram(s)sequenceDiagram
participant ManaAbilityReadiness
participant legality_simulation_is_redundant
participant GameState
ManaAbilityReadiness->>legality_simulation_is_redundant: evaluate cost and board guards
legality_simulation_is_redundant->>GameState: check reservations and CantPayCost statics
alt redundant
ManaAbilityReadiness-->>ManaAbilityReadiness: return readiness without cloning
else not redundant
ManaAbilityReadiness->>GameState: clone and simulate legality
GameState-->>ManaAbilityReadiness: return simulated result
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What this fixes
On a real 4-player Commander board (676 objects, 193 Treasure tokens)
derive_display_statetook 2233 ms per resolution. The mana-availability display sweep clones the wholeGameStateonce per mana source whose cost the cheap gate cannot conclusively decide — and Treasure's{T}, Sacrifice this: Add one mana of any coloris exactly that shape, becauseAbilityCost::Sacrificeis unconditionally uncovered byall_components_cheap_gate_covered.Cost was O(N² × battlefield):
layers_fullgrew exactly as N(N+1)/2, because each resolution adds a mana source that the next sweep re-clones.The change
One module-private predicate at the
mana_abilities.rsdecision seam widens the skip to whole-tree choice-free costs that sacrifice exactly the ability's own source, reusing the existingmana_sources::has_unambiguous_self_sacrifice_componentclassifier. Two conservative guards keep the answer identical:cost_sacrifices_reserved_sourcerather than re-deriving it.CantPayCoststatic is in play, because the readiness gate evaluatesplayer_cant_sacrifice_as_coston the pre-payment state while payment re-evaluates it after this tree's{T}component has already tapped the source.Both guards decline into today's exact simulation, so a spurious guard costs performance and never correctness.
The classifier is additionally bounded to a flat tree carrying exactly one
Sacrifice(SelfRef, Count{1})leaf and at most oneTapleaf. Multi-consuming-leaf trees would otherwise flip the answer in the unsafe direction: the payer flattens and pays one leaf at a time, so a second consuming leaf errors after the first already mutated the source.The flatness clause is required for soundness, not tidiness. A flattened census alone widens the classifier onto nested
Composite[Composite[Tap, Sac]], because the same one-level blindness sits upstream in the readiness gate'shas_tap_component— so a tapped source with a nested tree clears readiness, and removing the blindness on the classifier side alone would let it fast-pathtruewhile the payer errors on the flattened{T}. The resulting acceptance set is a strict subset of the previous one, so every consumer can only move toward pre-fix behavior.SacrificeRequirement::AggregateandCount{n>1}remain excluded by construction via the literalCount { count: 1 }pattern; no guard was added for them. No card-name or token-name matching.types/ability.rsis untouched.Measured result
Perf counters — deterministic, and what the committed tests assert — on a synthetic storm at six board sizes:
legality_cloneslayers_fullBase is exactly N(N+1)/2 on both counters; the candidate is exactly 0 at every size.
Clean back-to-back wall-clock on an idle machine, base vs candidate: ~4–5× on the large boards (50/200: 6755 ms → 1262 ms; 50/400: 10405 ms → 2538 ms). Small boards are noise-dominated single trials; the counters are the criterion.
Verification
clippy -D warnings,rustfmt --check— all green.card-data.jsonbyte-identical across base and candidate. No coverage impact.Known limitation, documented in code
The
CantPayCostguard keys on board-global static presence, not per-source. A single such permanent (Yasharn, Impeccable Sire) returns every mana source on the board to the simulation path, so the fix goes inert on that archetype. A narrowing that would keep tapless sources fast-pathed is filed as a follow-up rather than adopted here, because it depends on a hop that was not verified.Summary by CodeRabbit
Performance
Bug Fixes
Tests