fix(engine): Slaughter the Strong — keep a total-power-capped subset, sacrifice the rest (#4380)

[nickmopen]

Summary

Fixes #4380. "Each player chooses any number of creatures they control with total power 4 or less, then sacrifices all other creatures they control." Previously the keep-constraint was dropped and every other creature was sacrificed (the reported "it just kills all creatures").

Parser

• New total_power_cap selection mode on Effect::ChooseAndSacrificeRest. "any number of creatures they control with total power N or less" parses to the capped keep mode, and "sacrifices all other creatures they control" is absorbed by the choose-and-sacrifice followup (extended with a creature domain).
• Category cards (Cataclysm, Tragic Arrogance, Cataclysmic Gearhulk) are unchanged.

Runtime

• WaitingFor::KeepWithinTotalPowerChoice + GameAction::ChooseKeptCreatures drive a per-player (APNAP) subset choice, validated so the kept set's combined power does not exceed the cap.
• Keeping all eligible creatures auto-resolves when already within the cap (no needless prompt); the unchosen are sacrificed via the shared simultaneous sweep (sacrifice_unchosen), preserving co-departing leaves-the-battlefield observers.
• AI candidate generation offers the empty keep and a greedy max-keep within the cap.

Tests

Runtime regression: the caster is prompted; an over-cap kept subset is rejected; the kept subset survives while the rest are sacrificed; a player already within the cap auto-keeps.

Verification

cargo fmt, clippy -p engine --lib, and the parser-combinator gate are clean. Full engine lib suite (14139 passed, 0 failed) plus the new integration test pass.

[gemini-code-assist]

Code Review

This pull request implements support for total-power-capped keep effects (such as "Slaughter the Strong") where players choose any number of creatures they control with total power N or less and sacrifice the rest. This includes updates to the parser, game state, action types, AI candidate generation, and resolution logic, along with comprehensive integration tests. The code review feedback suggests refactoring step_total_power to group recursive arguments into a context struct for better readability, and optimizing the validation logic in handle_resolution_choice by using HashSet instead of Vec::contains to avoid quadratic complexity.

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.

[gemini-code-assist]

This function has a large number of arguments, which is acknowledged by #[allow(clippy::too_many_arguments)]. To improve maintainability and readability, consider grouping the arguments that are passed through recursively into a context struct. This would be especially beneficial for arguments that don't change during the recursion, like source_id, source_controller, chooser_scope, choose_filter, sacrifice_filter, cap, and scoped_players.

[gemini-code-assist]

The validation logic here uses Vec::contains inside a loop for both eligible and chosen lists, which has quadratic complexity (O(N*M)). For better performance and cleaner code, you could convert eligible to a HashSet before the loop for O(1) lookups, and use another HashSet to collect unique kept items.

Suggested change

	let mut chosen = Vec::new();
	for id in &kept {
	if !eligible.contains(id) {
	return Err(EngineError::InvalidAction(format!(
	"Creature {id:?} is not eligible to keep"
	)));
	}
	if !chosen.contains(id) {
	chosen.push(*id);
	}
	}
	let eligible_set: std::collections::HashSet<_> = eligible.iter().copied().collect();
	let mut chosen_set = std::collections::HashSet::with_capacity(kept.len());
	for id in &kept {
	if !eligible_set.contains(id) {
	return Err(EngineError::InvalidAction(format!(
	"Creature {id:?} is not eligible to keep"
	)));
	}
	chosen_set.insert(*id);
	}
	let chosen: Vec<_> = chosen_set.into_iter().collect();

[github-actions]

Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main e0eff39264c8)

2 card(s) · ability/ChooseAndSacrificeRest · added: ChooseAndSacrificeRest (targets=0+)

Examples: Destined Confrontation, Slaughter the Strong

2 card(s) · ability/TargetOnly · removed: TargetOnly (target=scoped player controls creature, targets=0+)

Examples: Destined Confrontation, Slaughter the Strong

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

[MED] Preserve source-controller provenance when recomputing eligible creatures. Evidence: crates/engine/src/game/effects/choose_and_sacrifice_rest.rs:311 builds FilterContext::from_source(state, source_id), while this waiting state already stores source_controller and the existing CategoryChoice resume path uses FilterContext::from_source_with_controller(source_id, controller) before recomputing eligibility. Why it matters: if this parameterized total-power mode is used with any controller-relative choose_filter, a resumed/serialized choice or missing source object can silently evaluate the eligibility filter with no controller and prompt/sacrifice the wrong set. Suggested fix: use FilterContext::from_source_with_controller(source_id, source_controller) in step_total_power, mirroring advance_to_next_player.

[matthewevans]

[MED] Total-power choices auto-keep all creatures when the rules still allow choosing fewer. Evidence: crates/engine/src/game/effects/choose_and_sacrifice_rest.rs:318-323 auto-keeps every eligible creature when their total power is already within the cap, and the regression locks that behavior in at crates/engine/tests/integration/slaughter_the_strong_total_power_4380.rs:95-97; CR 107.1c says “any number” may be zero. Why it matters: a player resolving Slaughter the Strong may intentionally choose fewer creatures, including zero, to sacrifice creatures even if their board already totals 4 power or less. Suggested fix: only auto-skip truly empty eligible sets; otherwise prompt for the subset, with UI/AI free to default to keeping all.

[MED] The new waiting state is missing from the resolution-continuation pause guard. Evidence: crates/engine/src/game/effects/mod.rs:1669-1732 lists the WaitingFor states that pause a resolving ability before its sub-ability/tail runs, but omits WaitingFor::KeepWithinTotalPowerChoice; the continuation install check at crates/engine/src/game/effects/mod.rs:6743-6753 depends on that helper. Why it matters: if this parameterized ChooseAndSacrificeRest mode is followed by any sub-ability/tail, that tail can run before the player answers the keep-within-power prompt. Suggested fix: add KeepWithinTotalPowerChoice beside CategoryChoice in waits_for_resolution_choice and add a chained-effect regression.

[matthewevans]

Backend re-reviewed current head ca992b00. The prior backend blockers I raised are addressed: source-controller provenance is preserved for the resumed eligibility check, within-cap choices still prompt so the player may keep fewer/zero creatures, and resolution continuations wait on KeepWithinTotalPowerChoice before draining the chained tail.

I am not approving or enqueueing from the sweep because this PR still has frontend/client UI surface. Leaving that acceptance to Matt.

[matthewevans]

@nickmopen please attach screenshots for all PRs that contain frontend changes

[matthewevans]

[MED] Manabrew snapshots do not mark the new keep-within-total-power eligible creatures as choosable. Evidence: crates/manabrew-compat/src/lib.rs:1326 handles the existing CategoryChoice sibling, but KeepWithinTotalPowerChoice falls through to _ => HashSet::new() at crates/manabrew-compat/src/lib.rs:1362; build_game_view consumes that set for card is_choosable at crates/manabrew-compat/src/lib.rs:433. Why it matters: the new player-facing prompt reaches Manabrew with no selectable/choosable eligible creatures, so Slaughter the Strong's new choice surface is incomplete in that adapter. Suggested fix: add a WaitingFor::KeepWithinTotalPowerChoice { player, eligible, .. } if *player == viewer => eligible.iter().copied().collect() arm and cover it in choosable_objects_includes_resolution_choice_siblings.

[matthewevans]

[MED] Add the Manabrew prompt/action path for KeepWithinTotalPowerChoice.

The latest head fixes the narrow choosable-object regression, but Manabrew still cannot actually present or answer the new Slaughter the Strong choice. Evidence: build_prompt in crates/manabrew-compat/src/lib.rs has no WaitingFor::KeepWithinTotalPowerChoice arm, so this state still falls through to UnsupportedPrompt; the PR only adds KeepWithinTotalPowerChoice to the choosable_objects highlighting helper. That means a Manabrew client reaching this prompt can see eligible cards marked choosable in the snapshot, but prompt construction fails before the player can submit which creatures to keep. Please add a prompt/action path for KeepWithinTotalPowerChoice, including the cap/eligible object ids and a way to submit arbitrary ChooseKeptCreatures subsets rather than relying on the unsupported fallback.

[matthewevans]

Backend/Manabrew re-review on current head 9f65b5e: the prior Manabrew blocker is addressed. KeepWithinTotalPowerChoice now has a prompt type with eligible object ids and cap, plus a direct KeepCreaturesDecision → GameAction::ChooseKeptCreatures translation and a round-trip regression.

I am not approving/enqueueing from the sweep because this PR still includes frontend/UI changes, so final frontend acceptance is deferred for maintainer handling.

[nickmopen]

@matthewevans sure — here are the frontend screenshots for this PR.

The frontend change is a new interactive choice modal, "Choose Creatures to Keep" (KeepWithinTotalPowerChoiceModal), shown when resolving Slaughter the Strong. Each player selects which of their creatures to keep within the total-power cap; the running total turns red and Confirm is disabled while the selection exceeds the cap.

1. Initial state — nothing selected yet (0 / 4):

2. Valid selection — Grizzly Bears (2) + Llanowar Elves (1) = 3 / 4, within the cap, Confirm enabled:

3. Over the cap — selecting Colossal Dreadmaw (6) pushes it to 6 / 4; counter turns red and Confirm is disabled:

[matthewevans]

Ok, I really like this PR. One thing I've been trying to get away from (when possible) is showing modals/dialogs when the battlefield itself is better suited for whatever the selection is. I'd ideally like to see this where the player can select permanents directly on the battlefield with a total power (or whatever metric it is) indicator that the user sees as they select creatures. We should have precedence for this with WaitingFor::Sacrifice.

[nickmopen]

@matthewevans makes sense — agreed that on-battlefield selection is the better UX here. I'll rework it to drop the modal and select directly on the battlefield, following the WaitingFor::Sacrifice precedent.

Concretely, the plan:

• Route KeepWithinTotalPowerChoice through the existing board-selection path (BoardChoiceSelection / BattlefieldSacrificeChoiceView in gameStateView.ts) instead of CardChoiceModal, so the eligible creatures become clickable on the battlefield and the player picks them in place.
• Remove the KeepWithinTotalPowerChoiceModal (and its CardChoiceModal switch entry).
• Add a live total-power indicator that updates as creatures are selected and blocks confirm once it exceeds the cap.

One small note: the existing board selections are count/range-based (exactCount / rangeCount), not metric-based, so this adds a "capped-by-metric" selection variant (power sum ≤ N) to carry the cap + running total. Engine side is unchanged — it already sends eligible + cap, and the ChooseKeptCreatures dispatch stays the same.

Does that approach sound right to you? If so I'll push the change.

[matthewevans]

@matthewevans makes sense — agreed that on-battlefield selection is the better UX here. I'll rework it to drop the modal and select directly on the battlefield, following the WaitingFor::Sacrifice precedent.

Concretely, the plan:

* Route `KeepWithinTotalPowerChoice` through the existing board-selection path (`BoardChoiceSelection` / `BattlefieldSacrificeChoiceView` in `gameStateView.ts`) instead of `CardChoiceModal`, so the eligible creatures become clickable on the battlefield and the player picks them in place.

* Remove the `KeepWithinTotalPowerChoiceModal` (and its `CardChoiceModal` switch entry).

* Add a live **total-power indicator** that updates as creatures are selected and blocks confirm once it exceeds the cap.

One small note: the existing board selections are count/range-based (exactCount / rangeCount), not metric-based, so this adds a "capped-by-metric" selection variant (power sum ≤ N) to carry the cap + running total. Engine side is unchanged — it already sends eligible + cap, and the ChooseKeptCreatures dispatch stays the same.

Does that approach sound right to you? If so I'll push the change.

Yes this sounds great if we're leveraging existing patterns. Thank you!

[matthewevans]

[MED] totalPowerAtMost uses positive-clamped power in the client, rejecting legal Slaughter keep sets with negative-power creatures. Evidence: client/src/viewmodel/gameStateView.ts:486 sums Math.max(obj?.power ?? 0, 0), while the engine's Slaughter total uses raw power via obj.power.unwrap_or(0) at crates/engine/src/game/effects/choose_and_sacrifice_rest.rs:265. Why it matters: the engine allows a 5-power creature plus a -1-power creature under cap 4, but the UI would count that as 5 and disable confirm. Suggested fix: use raw power for totalPowerAtMost while preserving positive-only contribution for Crew/Saddle-style totalPowerAtLeast.

[MED] Collapsed grouped permanents do not get the keep-within-total-power picker. Evidence: client/src/components/board/GroupedPermanent.tsx:40 lists waiting states that can open the collapsed group picker, but it omits KeepWithinTotalPowerChoice; GroupedPermanent.tsx:105 exits before getBoardChoiceView can expose the group choices. Why it matters: collapsed identical eligible creatures cannot open the quantity picker for Slaughter, so the battlefield selection UI can only toggle the representative object. Suggested fix: include KeepWithinTotalPowerChoice in waitingForPlayer and cover the collapsed grouped keep-choice path.

[matthewevans]

Backend/current-head re-review on 96b173aa: the prior Slaughter issues I raised are addressed. The client now uses raw power for totalPowerAtMost, collapsed grouped permanents route through KeepWithinTotalPowerChoice, and the backend/Manabrew paths still look clean.

I am leaving frontend acceptance deferred for Matt rather than approving/enqueueing from the sweep.

[nickmopen]

Frontend screenshots for the reworked, on-battlefield keep selection (per the request to drop the modal and select directly on the battlefield with a live total-power indicator, following the WaitingFor::Sacrifice precedent).

The eligible creatures are selectable in place on the battlefield (each shows a KEEP badge — no modal), with a running power X / cap indicator; Confirm disables the moment the kept set exceeds the cap.

1. Initial — nothing selected yet, "Keep: power 0 / 4 or less":

2. Valid selection — Grizzly Bears (2) + Llanowar Elves (1) = 3/4, within the cap, Confirm enabled:

3. Over the cap — adding Colossal Dreadmaw (6) → 9/4, Confirm disabled:

[matthewevans]

@nickmopen this looks good! In the second image, when you only have 2 creatures selected, I can't see anything indicating that they are selected and toggle-able? We need something to indicate what the current selection is.

[nickmopen]

Good catch — the selected vs. eligible-but-unselected states were only differing by a subtle glow intensity on the same ring, which was easy to miss (especially on art-less cards).

Updated so the current selection is unambiguous and reads as a toggle: selected creatures now get a ✓ on the badge plus a brighter, filled (inset) glow, while eligible-but-unselected creatures show the same badge dimmed. This applies to every multi-select board choice, not just Slaughter.

Here, only Grizzly Bears + Llanowar Elves are selected (3/4) — note the two ✓ KEEP badges vs. the two dimmed KEEP badges:

And over the cap (3 selected → 9/4, Confirm disabled):