[codex] Reclassify basic land mana backlog bucket - #5089
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors synthesize_basic_land_mana to group multiple basic land subtypes into a single color-choice producer (ManaProduction::AnyOneColor) when multiple subtypes are present, resolving backlog issue 29. It also adds corresponding unit tests and updates the parser misparse backlog documentation. The feedback suggests avoiding a redundant vector allocation by moving the colors vector directly when its length is 1.
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 produced = if colors.len() == 1 { | ||
| ManaProduction::Fixed { | ||
| colors: vec![colors[0]], | ||
| contribution: ManaContribution::Base, | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid redundant vector allocation.
Why it matters: When colors.len() == 1, colors is already a Vec<ManaColor> containing exactly one element. We can move it directly instead of allocating a new vector with vec![colors[0]] and dropping the old one.
Suggested fix: Pass colors directly.
let produced = if colors.len() == 1 {
ManaProduction::Fixed {
colors,
contribution: ManaContribution::Base,
}
}
Parse changes introduced by this PR · 86 card(s), 24 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Multi-basic lands are collapsed into one choice ability, but CR 305.6 grants one intrinsic ability per basic land type. Evidence: crates/engine/src/database/synthesis.rs now emits a single ManaProduction::AnyOneColor when colors.len() > 1, while existing engine code documents and tests the CR 305.6 model as separate intrinsic abilities: crates/engine/src/game/effects/effect.rs checks that a land gaining all basic land types gets one fixed tap ability for each color, and crates/engine/src/game/engine.rs treats dual lands/triomes as lands with multiple mana options selected by ability_index. Why it matters: card-data support for 86 typed lands would be marked fixed while changing the ability identity/count the engine exposes for dual lands and triomes; this is not just presentation, because ability indexing and ability inspection observe those abilities. Suggested fix: keep one synthesized {T}: Add <color> ability per basic land type at the engine/card-data ability layer, and solve the root #29 export/UI grouping at the consumer/display or mana-option normalization seam if duplicate same-cost options need to be presented as a color choice.
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head. This now keeps CR 305.6 behavior intact at the engine/card-data layer and limits the PR to removing the invalid backlog bucket / moving the remaining non-matching cards to uncategorized triage. The current local diff has no synthesis or parser code changes, so auto-merge can wait for required checks.
Summary
This PR now treats root #29 as a backlog false-positive bucket rather than an engine/card-data behavior bug.
Reviewer feedback pointed out that CR 305.6 grants a separate intrinsic mana ability for each basic land type. That means typed duals/triomes should continue to expose separate fixed
{T}: Add <color>abilities at the engine/card-data layer, because ability identity andability_indexobserve those rows.The PR therefore removes the invalid root #29 bucket from
docs/parser-misparse-backlog.mdinstead of changing synthesis behavior:Other / uncategorized misparsefor separate triage.There is no net engine or parser code change against current
origin/main.Validation
cargo fmt --allcargo test -p engine --lib basic_land_mana_synthesis_testspassed before the behavior change was reverted.git diff origin/main -- crates/engine/src/database/synthesis.rsis empty, so no parsed card-data change is expected from this PR.git diff --checkpasses.