feat(engine): support Ozai conditional mana static - #6795
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
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 (2)
📝 WalkthroughWalkthroughThe parser now supports “or more unspent mana” conditions. Layer evaluation resolves captured or current controllers for transient and non-transient effects, with updated consumers and tests covering mana gates, controller changes, combat assignments, and cleanup transitions. ChangesUnspent mana conditions
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OracleText
participant Parser
participant LayerSystem
participant GameState
OracleText->>Parser: parse unspent mana condition
Parser->>LayerSystem: lower conditional static effect
LayerSystem->>GameState: evaluate with selected controller
GameState->>LayerSystem: return keyword applicability
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/off_zone_characteristics.rs (1)
176-191: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the transient controller in
crates/engine/src/game/sba.rs#L1530-L1536.matches_target_filterstill builds its context fromFilterContext::from_source(...), so a transient grant can evaluate the recipient set under a different controller thanactive_effect_condition_controller(...)uses for the condition. Mirror the layer path here and thread the same controller into both checks.🤖 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/off_zone_characteristics.rs` around lines 176 - 191, The off-zone recipient filter and condition use different controllers for transient grants. In crates/engine/src/game/off_zone_characteristics.rs lines 176-191, reuse the controller from active_effect_condition_controller for FilterContext::from_source_with_controller and the condition evaluation; in crates/engine/src/game/sba.rs lines 1530-1536, update matches_target_filter to build its context with that same transient controller so both checks agree.
🧹 Nitpick comments (2)
crates/engine/src/game/layers.rs (2)
5985-5994: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetained non-recipient conditions are now re-evaluated per candidate object on the layer hot path.
Because printed/granted statics no longer gate at collection time (Line 4306, Line 4471, Line 5092), every retained condition is evaluated inside the affected-set filter — once per candidate, per effect, per pass. For a board-wide
affectedfilter with a condition that itself scans state (IsPresent { filter },QuantityComparisonoverObjectCount), this turns one gather-time evaluation into O(candidates) evaluations.Deferring the evaluation is required for the layer-2 controller fix, but the result is invariant across recipients whenever
condition_uses_recipient_context(condition)is false — that case can be evaluated once per effect (after the controller is resolved) and reused for the whole candidate scan, preserving the new semantics.🤖 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/layers.rs` around lines 5985 - 5994, Update the affected-set filtering around evaluate_condition_with_recipient to evaluate non-recipient-dependent conditions once per effect after resolving the controller, then reuse that result for every candidate; retain per-candidate evaluation for conditions where condition_uses_recipient_context(condition) is true, preserving the deferred layer-2 controller semantics.
922-953: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the two identical controller-selection helpers.
active_effect_condition_controllerandcombat_effect_condition_controllerhave byte-identical bodies and differ only in the effect type. A single private helper over the three fields keeps the CR 109.5 / CR 611.2c rule in one place, so a future change to the discrimination can't drift between the object path and the combat path.♻️ Suggested shared helper
+fn condition_controller( + state: &GameState, + transient_id: Option<u64>, + controller: PlayerId, + source_id: ObjectId, +) -> PlayerId { + if transient_id.is_some() { + controller + } else { + state + .objects + .get(&source_id) + .map_or(controller, |source| source.controller) + } +} + pub(crate) fn active_effect_condition_controller( state: &GameState, effect: &ActiveContinuousEffect, ) -> PlayerId { - if effect.transient_id.is_some() { - effect.controller - } else { - state - .objects - .get(&effect.source_id) - .map_or(effect.controller, |source| source.controller) - } + condition_controller(state, effect.transient_id, effect.controller, effect.source_id) } fn combat_effect_condition_controller( state: &GameState, effect: &ActiveCombatAssignmentRuleEffect, ) -> PlayerId { - if effect.transient_id.is_some() { - effect.controller - } else { - state - .objects - .get(&effect.source_id) - .map_or(effect.controller, |source| source.controller) - } + condition_controller(state, effect.transient_id, effect.controller, effect.source_id) }🤖 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/layers.rs` around lines 922 - 953, Collapse active_effect_condition_controller and combat_effect_condition_controller into one private shared helper that accepts the needed transient_id, controller, and source_id values, preserving the existing transient-versus-source-controller selection behavior. Update both call sites to use the shared helper and remove the duplicate function bodies.
🤖 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/tests/integration/ozai_phoenix_king_unspent_mana.rs`:
- Line 11: Extend the Ozai integration test around OZAI_ORACLE to advance
through a cleanup step with unspent mana and assert that the mana is converted
to red, while also verifying Ozai’s six-mana flying/indestructible condition at
the boundary. Preserve the existing casting-spend assertions and use the test’s
established game-step and mana assertion helpers.
---
Outside diff comments:
In `@crates/engine/src/game/off_zone_characteristics.rs`:
- Around line 176-191: The off-zone recipient filter and condition use different
controllers for transient grants. In
crates/engine/src/game/off_zone_characteristics.rs lines 176-191, reuse the
controller from active_effect_condition_controller for
FilterContext::from_source_with_controller and the condition evaluation; in
crates/engine/src/game/sba.rs lines 1530-1536, update matches_target_filter to
build its context with that same transient controller so both checks agree.
---
Nitpick comments:
In `@crates/engine/src/game/layers.rs`:
- Around line 5985-5994: Update the affected-set filtering around
evaluate_condition_with_recipient to evaluate non-recipient-dependent conditions
once per effect after resolving the controller, then reuse that result for every
candidate; retain per-candidate evaluation for conditions where
condition_uses_recipient_context(condition) is true, preserving the deferred
layer-2 controller semantics.
- Around line 922-953: Collapse active_effect_condition_controller and
combat_effect_condition_controller into one private shared helper that accepts
the needed transient_id, controller, and source_id values, preserving the
existing transient-versus-source-controller selection behavior. Update both call
sites to use the shared helper and remove the duplicate function bodies.
🪄 Autofix (Beta)
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: 42245289-1a46-4fa3-8591-1b021e51804e
📒 Files selected for processing (7)
crates/engine/src/game/layers.rscrates/engine/src/game/off_zone_characteristics.rscrates/engine/src/game/sba.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/ozai_phoenix_king_unspent_mana.rs
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/layers.rs`:
- Around line 950-966: Add a verified CR 109.5 citation to the documentation
comment directly above condition_controller, keeping the existing explanation of
the “you”/“your” resolution behavior and leaving the function implementation
unchanged.
🪄 Autofix (Beta)
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: 2aa1cfe3-c8a3-4c53-93f4-7068efa36c15
📒 Files selected for processing (4)
crates/engine/src/game/layers.rscrates/engine/src/game/off_zone_characteristics.rscrates/engine/src/game/sba.rscrates/engine/tests/integration/ozai_phoenix_king_unspent_mana.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
MED — This transient combat-assignment path loses the controller snapshot needed for controller-relative filters.
At crates/engine/src/game/layers.rs:4966, the effect builds FilterContext::from_source(...), which reads the source's current controller. The adjacent condition evaluation (lines 4972–4978) correctly uses the captured controller. If the source changes control after the assignment is created, a condition such as “creatures you control” can therefore apply to the new controller’s creatures instead of the controller that created the assignment.
Please compute the condition controller once and pass it to FilterContext::from_source_with_controller(...). Extend the test beyond SpecificObject: exercise ControllerRef::You with P0/P1, asserting that P0’s intended recipients receive the effect and P1’s do not after the source is stolen.
Please also audit the likely copied branch at layers.rs:7134/7142 for the same snapshot/live-controller mismatch.
matthewevans
added
enhancement
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review complete: controller provenance and runtime regression coverage are sound.
2 participants
Summary
Validation
Validation note
The local frontend lint could not start because pnpm requires build-script approval while bootstrapping dependencies. No client code is changed; GitHub CI remains authoritative for this check.
Summary by CodeRabbit