fix(engine): classify City-of-Brass-shaped self-tap damage into mana-source penalty (#5912)#6033
Conversation
…source penalty (phase-rs#5912) City of Brass and similar lands print their self-damage as a separate "Whenever this land becomes tapped, it deals 1 damage to you." trigger (TriggerMode::Taps), not folded into the "{T}: Add one mana of any color." ability's own resolution chain the way painlands do. mana_ability_penalty only walked the mana ability's own chain, so it classified these lands byte-identically to a basic land (ManaSourcePenalty::None) and auto-tap could pick one over a truly free source (Island) for the same slot. object_mana_ability_penalty merges the existing ability-chain classification with a scan of the object's own self-referential Taps triggers, and now backs auto-tap sort, the priority-meaningful gate, and UntapLandForMana undoability so the three consumers cannot drift.
4a2b951 to
066f9bf
Compare
![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 addresses Issue #5912 by ensuring that mana abilities with sibling 'taps' triggers, such as City of Brass, are correctly classified for auto-tap and priority logic. It introduces a new authoritative function, object_mana_ability_penalty, which aggregates penalties from both the ability's own resolution chain and any self-referential sibling triggers. The engine, AI support, and auto-tap logic have been updated to utilize this function, and comprehensive regression tests have been added to verify the correct classification of these land types. The review comment suggests a refactoring of the new logic to improve idiomatic Rust usage and avoid Clippy lints.
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.
| let mut found = false; | ||
| let mut total = Some(0_u16); | ||
| for trigger in obj.trigger_definitions.iter_all() { | ||
| if trigger.mode != TriggerMode::Taps { | ||
| continue; | ||
| } | ||
| let self_referential = match &trigger.valid_card { | ||
| None => true, | ||
| Some(filter) => super::trigger_matchers::target_filter_matches_object( | ||
| state, object_id, filter, object_id, | ||
| ), | ||
| }; | ||
| if !self_referential { | ||
| continue; | ||
| } | ||
| let Some(execute) = trigger.execute.as_deref() else { | ||
| continue; | ||
| }; | ||
| if let Some(amount) = chain_harms_controller_amount(execute) { | ||
| found = true; | ||
| total = fold_amount(total, amount); | ||
| } | ||
| } | ||
| found.then_some(total) |
There was a problem hiding this comment.
This function's implementation can be refactored to be more concise and idiomatic by using a functional approach with iterators. Please use is_none_or instead of map_or(true, ...) to comply with the repository's Rust 1.82+ style guidelines and avoid Clippy's unnecessary_map_or lint.
let harm_amounts = obj
.trigger_definitions
.iter_all()
.filter(|trigger| trigger.mode == TriggerMode::Taps)
.filter(|trigger| {
trigger.valid_card.as_ref().is_none_or(|filter| {
super::trigger_matchers::target_filter_matches_object(
state,
object_id,
filter,
object_id,
)
})
})
.filter_map(|trigger| trigger.execute.as_deref())
.filter_map(chain_harms_controller_amount);
let mut harm_amounts_peekable = harm_amounts.peekable();
if harm_amounts_peekable.peek().is_none() {
None
} else {
Some(harm_amounts_peekable.fold(Some(0), fold_amount))
}References
- Do not replace Option::is_none_or with map_or(true, ...) in Rust codebases that support Rust 1.82.0 or newer, as this will trigger Clippy's unnecessary_map_or lint.
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.
Maintainer follow-up for 5625e3c3b6c0a07b82af0bc3fdfafb3d3e2087ba:
- added the tap-cost guard so a no-tap mana ability cannot inherit a sibling
TriggerMode::Tapspenalty; - added the discriminating City-of-Brass-shaped no-tap regression; and
- corrected the Tarnished Citadel explanation (its colored mana ability embeds its damage).
The change is held solely for the current required CI and the current-head parse-diff baseline (Baseline pending). The local Tilt instance watches a different checkout, so it cannot serve as verification for this worktree; no direct build was run. This is a status hold, not a request for contributor changes.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head maintainer review is clean.
- The object-level helper now correctly charges City of Brass's sibling
TriggerMode::Tapstrigger, and the engine-level auto-tap regression distinguishes it from the prior ability-only classifier. - The no-tap guard prevents unrelated mana abilities from inheriting the object's tap-trigger penalty.
- Current parse-diff reports no parser changes and required CI is green.
Address Gemini review nit on PR phase-rs#6033: replace the imperative found/total loop with a filter/filter_map iterator chain using is_none_or, matching the repo's stated preference for iterator methods over manual loops. Behavior unchanged — same two City-of-Brass unit tests and the auto-tap end-to-end regression test still pass.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head maintainer re-review is clean.
The author’s iterator rewrite preserves the prior sibling-trigger classifier: it filters only Taps triggers whose card filter matches the tapped source, skips missing execution chains, sums fixed harms, and retains an unknown amount as None. The City-of-Brass runtime regression and no-tap guard continue to discriminate the behavior. The current parser-impact evidence remains no card-parse changes.
2 participants
Summary
Closes #5912.
City of Brass (and reprints like Tarnished Citadel) print their self-damage
as a separate triggered ability — "Whenever this land becomes tapped, it
deals 1 damage to you." (
TriggerMode::Taps) — rather than folding thedamage into the
{T}: Add one mana of any color.ability's own resolutionchain the way painlands do (Adarkar Wastes: "{T}: Add {W} or {U}. This land
deals 1 damage to you." is a single ability).
mana_ability_penaltyonly walks the mana ability's own effect chain(
ability.effect/sub_ability/else_ability), so it never sees a siblingTriggerDefinitionon the same object. That misclassified City of Brass asManaSourcePenalty::None— byte-identical to a basic land — so auto-tap hadno reason to prefer a truly free source (Island) over it for a shared
generic-mana slot. Reported case: casting a
{1}{G}spell with City ofBrass + Llanowar Elves in play tapped City of Brass (1 damage) instead of
Island, even though Island could pay the generic cost for free.
object_mana_ability_penalty(mana_sources.rs) merges the existingability-chain classification with a scan of the object's own
self-referential
Tapstriggers (object_self_tap_harm_amount), reusing thesame
chain_harms_controller_amountwalk a painland's embedded damagealready goes through. All three real consumers of the penalty classification
now route through it instead of the ability-only
mana_ability_penalty, sothe fix can't drift between them:
scan_mana_abilities(auto-tap source sort — the reported bug)activate_ability_is_meaningful_priority(priority-meaningful gate)UntapLandForManaundoability check inengine.rs(a City-of-Brasstap was previously misclassified as undoable even though its damage
trigger had already resolved)
Files changed
crates/engine/src/game/mana_sources.rs—object_self_tap_harm_amount,object_mana_ability_penalty, wired intoscan_mana_abilities; two newunit tests.
crates/engine/src/game/casting_costs.rs— new end-to-end auto-tapregression test.
crates/engine/src/ai_support/mod.rs—activate_ability_is_meaningful_prioritynow calls
object_mana_ability_penalty.crates/engine/src/game/engine.rs—UntapLandForManaundo check nowcalls
object_mana_ability_penalty.CR references
classification authority for the penalty axis.
event, not on persisting state).
chain_harms_controller_amountalready classifies for painlands.Verification
cargo fmt --all— clean.cargo clippy -p engine --all-targets --features engine/proptest -- -D warnings— 0 warnings.cargo test -p engine --lib— 16754 passed, 0 failed, 7 ignored.cargo test -p engine(integration suite) — 3208 passed, 0 failed, 2 ignored.game::mana_sources::tests::city_of_brass_ability_alone_misclassifies_as_none(reach-guard: proves the ability-only classifier alone still returns
None, i.e. the gap this PR closes).game::mana_sources::tests::object_mana_ability_penalty_sees_city_of_brass_self_tap_damage_triggergame::casting_costs::tests::auto_tap_prefers_free_land_over_city_of_brass_self_damage_trigger(drives the real
auto_tap_mana_sourcespipeline; fails against thepre-fix code because auto-tap would tap City of Brass and pay 1 life).
./scripts/check-parser-combinators.sh origin/main—Gate A PASS head=d5f81e071168fad4ade8489dfce50003d56ec60b base=3b52c67a9025cd20da8b68243667eb8cdad76060(no parser files touched bythis change).
Anchored on
crates/engine/src/game/mana_sources.rs:378(chain_harms_controller_amount) —reused unmodified for the sibling trigger's
executechain.crates/engine/src/game/casting_costs.rs:13487(
auto_tap_prefers_non_life_mana_sources_when_equivalent) — existingend-to-end auto-tap regression pattern the new test mirrors.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Claimed parse impact
None — no parser files touched; this is a runtime/AI-support classification
fix only.
Closes #6020