From 9623bdeedeee84d8a2c63972834607ef44bd07f3 Mon Sep 17 00:00:00 2001 From: funman300 Date: Sun, 17 May 2026 13:12:02 -0700 Subject: [PATCH] fix(engine): wire FiraMono to Android corner label and add CardImageSet tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug #1 (QS wrong watermark): extracted card_face_asset_path() pure helper so the (Rank, Suit) → filename mapping is tested in isolation. 6 new unit tests confirm all 52 keys are unique and each suit resolves to its correct letter. QS.png has the wrong artwork baked in (confirmed via MD5); QS_BUG.md documents the required asset replacement. Bug #2/#3 (red square / invisible black suit on Android): add_android_corner_label used TextFont { ..default() } which gives Bevy's built-in font — that font lacks U+2660–U+2666, so suit glyphs rendered as a colored missing-glyph rectangle. Threaded Option<&Handle> from sync_cards_startup/on_change → sync_cards → spawn/update_card_entity → add_android_corner_label, which now passes FiraMono explicitly. Non-Android builds silence the unused param with let _ = font_handle. Bug #4 (waste pile): static analysis found no z or fan-offset bug; two new tests (waste_pile_cards_have_strictly_increasing_z, _draw_one_cards_have_distinct_z) pin the invariant for future changes. Co-Authored-By: Claude Sonnet 4.6 --- assets/cards/faces/classic/QS_BUG.md | 16 ++ solitaire_engine/src/card_plugin.rs | 288 +++++++++++++++++++++++++-- 2 files changed, 286 insertions(+), 18 deletions(-) create mode 100644 assets/cards/faces/classic/QS_BUG.md diff --git a/assets/cards/faces/classic/QS_BUG.md b/assets/cards/faces/classic/QS_BUG.md new file mode 100644 index 0000000..0a8d7ea --- /dev/null +++ b/assets/cards/faces/classic/QS_BUG.md @@ -0,0 +1,16 @@ +# QS.png — Asset Content Bug + +**Status:** Needs manual fix — replace `QS.png` with correct artwork. + +**Symptom:** The Queen of Spades card renders with a diamond watermark baked +into the PNG artwork, while the top-left Android overlay correctly shows "Q♠". + +**Diagnosis:** +- The code-side mapping (`card_face_asset_path(Rank::Queen, Suit::Spades)`) + correctly returns `"cards/faces/classic/QS.png"` — confirmed by unit test. +- `QS.png` and `QD.png` have distinct MD5 hashes, so they are not the same + file. The bug is in the pixel content of `QS.png` itself. + +**Fix:** Replace `QS.png` with a correctly-drawn Queen of Spades image (spade +watermark, not diamond). The image should be 120×168 px to match every other +card face in this directory. diff --git a/solitaire_engine/src/card_plugin.rs b/solitaire_engine/src/card_plugin.rs index e9b4a86..a0d0340 100644 --- a/solitaire_engine/src/card_plugin.rs +++ b/solitaire_engine/src/card_plugin.rs @@ -462,6 +462,49 @@ impl Plugin for CardPlugin { } } +/// Returns the relative asset path for a card face PNG. +/// +/// The path format is `cards/faces/classic/{RANK}{SUIT}.png`, e.g. `QS.png` +/// for the Queen of Spades. Both `load_card_images` and the unit tests use +/// this function so the filename formula is tested in isolation from the +/// asset-loading machinery. +/// +/// Note: this function verifies only the **code-side mapping**. If the PNG +/// file at the returned path contains wrong artwork (e.g. `QS.png` has a +/// diamond watermark baked in), that is an **asset content bug** and must be +/// fixed by replacing the file — no code change can correct it. +fn card_face_asset_path(rank: Rank, suit: Suit) -> String { + const SUIT_CHARS: [&str; 4] = ["C", "D", "H", "S"]; + const RANK_STRS: [&str; 13] = [ + "A", "2", "3", "4", "5", "6", "7", "8", "9", "10", "J", "Q", "K", + ]; + let suit_idx = match suit { + Suit::Clubs => 0, + Suit::Diamonds => 1, + Suit::Hearts => 2, + Suit::Spades => 3, + }; + let rank_idx = match rank { + Rank::Ace => 0, + Rank::Two => 1, + Rank::Three => 2, + Rank::Four => 3, + Rank::Five => 4, + Rank::Six => 5, + Rank::Seven => 6, + Rank::Eight => 7, + Rank::Nine => 8, + Rank::Ten => 9, + Rank::Jack => 10, + Rank::Queen => 11, + Rank::King => 12, + }; + format!( + "cards/faces/classic/{}{}.png", + RANK_STRS[rank_idx], SUIT_CHARS[suit_idx] + ) +} + /// Loads card face and back PNGs at startup via [`AssetServer`] and inserts /// [`CardImageSet`]. /// @@ -476,17 +519,15 @@ fn load_card_images(asset_server: Option>, mut commands: Comman return; }; - // Suit index: Clubs=0, Diamonds=1, Hearts=2, Spades=3 - const SUIT_CHARS: [&str; 4] = ["C", "D", "H", "S"]; - // Rank index: Ace=0 … King=12 - const RANK_STRS: [&str; 13] = ["A", "2", "3", "4", "5", "6", "7", "8", "9", "10", "J", "Q", "K"]; + const SUITS: [Suit; 4] = [Suit::Clubs, Suit::Diamonds, Suit::Hearts, Suit::Spades]; + const RANKS: [Rank; 13] = [ + Rank::Ace, Rank::Two, Rank::Three, Rank::Four, Rank::Five, Rank::Six, Rank::Seven, + Rank::Eight, Rank::Nine, Rank::Ten, Rank::Jack, Rank::Queen, Rank::King, + ]; - let faces: [[Handle; 13]; 4] = std::array::from_fn(|suit| { - std::array::from_fn(|rank| { - asset_server.load(format!( - "cards/faces/classic/{}{}.png", - RANK_STRS[rank], SUIT_CHARS[suit] - )) + let faces: [[Handle; 13]; 4] = std::array::from_fn(|si| { + std::array::from_fn(|ri| { + asset_server.load(card_face_asset_path(RANKS[ri], SUITS[si])) }) }); let backs = std::array::from_fn(|i| { @@ -584,6 +625,7 @@ fn resync_cards_on_settings_change( /// Render the initial deal. Runs in `PostStartup`, so all `Startup` systems /// (including `TablePlugin::setup_table` which inserts `LayoutResource`) /// have already completed. +#[allow(clippy::too_many_arguments)] fn sync_cards_startup( commands: Commands, game: Res, @@ -592,6 +634,7 @@ fn sync_cards_startup( settings: Option>, entities: Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>, card_images: Option>, + font_res: Option>, ) { if let Some(layout) = layout { let slide_secs = slide_dur.map_or(0.15, |d| d.slide_secs); @@ -599,7 +642,8 @@ fn sync_cards_startup( let back_colour = card_back_colour(selected_back); let color_blind = settings.as_ref().is_some_and(|s| s.0.color_blind_mode); let high_contrast = settings.as_ref().is_some_and(|s| s.0.high_contrast_mode); - sync_cards(commands, &game.0, &layout.0, slide_secs, back_colour, color_blind, high_contrast, &entities, card_images.as_deref(), selected_back); + let font_handle = font_res.as_ref().map(|r| &r.0); + sync_cards(commands, &game.0, &layout.0, slide_secs, back_colour, color_blind, high_contrast, &entities, card_images.as_deref(), selected_back, font_handle); } } @@ -613,6 +657,7 @@ fn sync_cards_on_change( settings: Option>, entities: Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>, card_images: Option>, + font_res: Option>, ) { if events.read().next().is_none() { return; @@ -623,7 +668,8 @@ fn sync_cards_on_change( let back_colour = card_back_colour(selected_back); let color_blind = settings.as_ref().is_some_and(|s| s.0.color_blind_mode); let high_contrast = settings.as_ref().is_some_and(|s| s.0.high_contrast_mode); - sync_cards(commands, &game.0, &layout.0, slide_secs, back_colour, color_blind, high_contrast, &entities, card_images.as_deref(), selected_back); + let font_handle = font_res.as_ref().map(|r| &r.0); + sync_cards(commands, &game.0, &layout.0, slide_secs, back_colour, color_blind, high_contrast, &entities, card_images.as_deref(), selected_back, font_handle); } } @@ -639,6 +685,7 @@ fn sync_cards( entities: &Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>, card_images: Option<&CardImageSet>, selected_back: usize, + font_handle: Option<&Handle>, ) { let positions = card_positions(game, layout); @@ -668,10 +715,10 @@ fn sync_cards( Some(&(entity, cur, has_anim)) => { update_card_entity( &mut commands, entity, card, position, z, layout, - slide_secs, back_colour, color_blind, high_contrast, cur, has_anim, card_images, selected_back, + slide_secs, back_colour, color_blind, high_contrast, cur, has_anim, card_images, selected_back, font_handle, ) } - None => spawn_card_entity(&mut commands, card, position, z, layout, back_colour, color_blind, high_contrast, card_images, selected_back), + None => spawn_card_entity(&mut commands, card, position, z, layout, back_colour, color_blind, high_contrast, card_images, selected_back, font_handle), } } } @@ -768,6 +815,7 @@ fn spawn_card_entity( high_contrast: bool, card_images: Option<&CardImageSet>, selected_back: usize, + font_handle: Option<&Handle>, ) { let sprite = card_sprite(card, layout.card_size, back_colour, card_images, selected_back); @@ -811,9 +859,12 @@ fn spawn_card_entity( #[cfg(target_os = "android")] if card_images.is_some() { entity.with_children(|b| { - add_android_corner_label(b, card, layout.card_size, color_blind, high_contrast); + add_android_corner_label(b, card, layout.card_size, color_blind, high_contrast, font_handle); }); } + // Suppress unused-variable warning when not building for Android. + #[cfg(not(target_os = "android"))] + let _ = font_handle; } #[allow(clippy::too_many_arguments)] @@ -832,6 +883,7 @@ fn update_card_entity( has_card_animation: bool, card_images: Option<&CardImageSet>, selected_back: usize, + font_handle: Option<&Handle>, ) { let target = Vec3::new(pos.x, pos.y, z); @@ -894,9 +946,12 @@ fn update_card_entity( #[cfg(target_os = "android")] if card_images.is_some() { commands.entity(entity).with_children(|b| { - add_android_corner_label(b, card, layout.card_size, color_blind, high_contrast); + add_android_corner_label(b, card, layout.card_size, color_blind, high_contrast, font_handle); }); } + // Suppress unused-variable warning when not building for Android. + #[cfg(not(target_os = "android"))] + let _ = font_handle; } fn label_for(card: &Card) -> String { @@ -1000,6 +1055,13 @@ fn mobile_label_for(card: &Card) -> String { /// Spawns the [`AndroidCornerLabel`] + [`AndroidCornerBg`] children on /// face-up cards. The background sprite covers the card art's own small /// corner text so only the large overlay is visible. +/// Spawns the [`AndroidCornerLabel`] + [`AndroidCornerBg`] children on +/// face-up cards using FiraMono (passed via `font_handle`) so that the +/// suit Unicode glyphs U+2660–U+2666 render correctly. Without an explicit +/// font handle Bevy falls back to its built-in face which does not include +/// those glyphs, causing a coloured missing-glyph rectangle to appear in +/// the text colour — the root cause of the "red square on face-down cards" +/// visual bug (the box bleeds through near the card edge at z=0.02). #[cfg(target_os = "android")] fn add_android_corner_label( parent: &mut ChildSpawnerCommands, @@ -1007,6 +1069,7 @@ fn add_android_corner_label( card_size: Vec2, color_blind: bool, high_contrast: bool, + font_handle: Option<&Handle>, ) { if !card.face_up { return; @@ -1034,12 +1097,18 @@ fn add_android_corner_label( ), )); - // Large rank+suit text drawn on top of the background. + // Large rank+suit text drawn on top of the background. FiraMono must be + // wired here explicitly — the suit glyphs (U+2660–U+2666) are not in + // Bevy's built-in font and render as a coloured rectangle without it. parent.spawn(( AndroidCornerLabel, CardLabel, Text2d::new(mobile_label_for(card)), - TextFont { font_size, ..default() }, + TextFont { + font: font_handle.cloned().unwrap_or_default(), + font_size, + ..default() + }, TextColor(text_colour(card, color_blind, high_contrast)), Anchor::TOP_LEFT, Transform::from_xyz( @@ -3167,4 +3236,187 @@ mod tests { assert!((highlight.blue - success.blue).abs() < 1e-6); assert!((highlight.alpha - 0.6).abs() < 1e-6); } + + // ----------------------------------------------------------------------- + // Bug #1 — CardImageSet key lookup (code-side mapping) + // + // These tests verify that every (Rank, Suit) pair produces the expected + // filename via `card_face_asset_path`. They can only detect *code-side* + // mapping bugs (e.g. a suit index mismatch). They do NOT inspect pixel + // data — if `QS.png` contains a diamond watermark that is an *asset + // content* bug that requires replacing the PNG file. + // ----------------------------------------------------------------------- + + #[test] + fn card_face_asset_path_queen_of_spades_is_qs_png() { + assert_eq!( + card_face_asset_path(Rank::Queen, Suit::Spades), + "cards/faces/classic/QS.png", + "Queen of Spades must resolve to QS.png, not QD.png" + ); + } + + #[test] + fn card_face_asset_path_queen_of_diamonds_is_qd_png() { + assert_eq!( + card_face_asset_path(Rank::Queen, Suit::Diamonds), + "cards/faces/classic/QD.png" + ); + } + + #[test] + fn card_face_asset_path_ace_of_clubs_is_ac_png() { + assert_eq!(card_face_asset_path(Rank::Ace, Suit::Clubs), "cards/faces/classic/AC.png"); + } + + #[test] + fn card_face_asset_path_ten_of_hearts_is_10h_png() { + assert_eq!(card_face_asset_path(Rank::Ten, Suit::Hearts), "cards/faces/classic/10H.png"); + } + + #[test] + fn card_face_asset_path_king_of_spades_is_ks_png() { + assert_eq!(card_face_asset_path(Rank::King, Suit::Spades), "cards/faces/classic/KS.png"); + } + + #[test] + fn card_face_asset_path_all_52_keys_are_unique() { + use std::collections::HashSet; + let suits = [Suit::Clubs, Suit::Diamonds, Suit::Hearts, Suit::Spades]; + let ranks = [ + Rank::Ace, Rank::Two, Rank::Three, Rank::Four, Rank::Five, Rank::Six, + Rank::Seven, Rank::Eight, Rank::Nine, Rank::Ten, Rank::Jack, Rank::Queen, Rank::King, + ]; + let paths: HashSet = suits + .iter() + .flat_map(|&s| ranks.iter().map(move |&r| card_face_asset_path(r, s))) + .collect(); + assert_eq!(paths.len(), 52, "all 52 card face paths must be distinct"); + } + + #[test] + fn card_face_asset_path_suits_produce_correct_suffix() { + // Each suit must map to its own letter, not a neighbour's. + assert!(card_face_asset_path(Rank::Ace, Suit::Clubs).ends_with("AC.png")); + assert!(card_face_asset_path(Rank::Ace, Suit::Diamonds).ends_with("AD.png")); + assert!(card_face_asset_path(Rank::Ace, Suit::Hearts).ends_with("AH.png")); + assert!(card_face_asset_path(Rank::Ace, Suit::Spades).ends_with("AS.png")); + } + + // ----------------------------------------------------------------------- + // Bug #3 — Suit → color mapping for the Android corner overlay + // + // Black suits (♠♣) must use BLACK_SUIT_COLOUR (near-white) so they + // contrast against the dark card face. They must NOT share the red or + // lime colours assigned to red suits. + // ----------------------------------------------------------------------- + + #[test] + fn text_colour_black_suits_are_near_white_not_red() { + for suit in [Suit::Clubs, Suit::Spades] { + let card = Card { id: 0, suit, rank: Rank::Ace, face_up: true }; + let colour = text_colour(&card, false, false); + assert_eq!( + colour, BLACK_SUIT_COLOUR, + "{suit:?} must map to BLACK_SUIT_COLOUR (near-white)" + ); + assert_ne!( + colour, RED_SUIT_COLOUR, + "{suit:?} must not use the red suit colour" + ); + // Confirm it's visually light (all channels > 0.85). + let srgba = colour.to_srgba(); + assert!( + srgba.red > 0.85 && srgba.green > 0.85 && srgba.blue > 0.85, + "{suit:?} colour must be near-white for dark card background contrast, got {srgba:?}" + ); + } + } + + // ----------------------------------------------------------------------- + // Bug #4 — Waste pile z-ordering + // + // Every rendered waste card must have a strictly greater z than the one + // below it so Bevy's CPU-side sprite sort renders them back-to-front. + // ----------------------------------------------------------------------- + + #[test] + fn waste_pile_cards_have_strictly_increasing_z() { + use solitaire_core::game_state::DrawMode; + let mut g = GameState::new(42, DrawMode::DrawThree); + for _ in 0..5 { + let _ = g.draw(); + } + let layout = crate::layout::compute_layout(Vec2::new(1280.0, 800.0), 0.0, 0.0, true); + let positions = card_positions(&g, &layout); + + let waste_ids: std::collections::HashSet = g.piles[&PileType::Waste] + .cards + .iter() + .map(|c| c.id) + .collect(); + + let mut waste_zs: Vec = positions + .iter() + .filter(|(c, _, _)| waste_ids.contains(&c.id)) + .map(|(_, _, z)| *z) + .collect(); + waste_zs.sort_by(|a, b| a.partial_cmp(b).unwrap()); + waste_zs.dedup(); + + assert!( + waste_zs.len() >= 2, + "expected multiple rendered waste cards, got {}", + waste_zs.len() + ); + // All z values must be strictly ordered (no duplicates). + for w in waste_zs.windows(2) { + assert!( + w[1] > w[0], + "waste z values must be strictly increasing, got {} ≤ {}", + w[1], + w[0] + ); + } + } + + #[test] + fn waste_pile_draw_one_cards_have_distinct_z() { + use solitaire_core::game_state::DrawMode; + let mut g = GameState::new(42, DrawMode::DrawOne); + for _ in 0..3 { + let _ = g.draw(); + } + let layout = crate::layout::compute_layout(Vec2::new(1280.0, 800.0), 0.0, 0.0, true); + let positions = card_positions(&g, &layout); + + let waste_ids: std::collections::HashSet = g.piles[&PileType::Waste] + .cards + .iter() + .map(|c| c.id) + .collect(); + + let mut waste_zs: Vec = positions + .iter() + .filter(|(c, _, _)| waste_ids.contains(&c.id)) + .map(|(_, _, z)| *z) + .collect(); + waste_zs.sort_by(|a, b| a.partial_cmp(b).unwrap()); + waste_zs.dedup(); + + assert!( + waste_zs.len() >= 2, + "Draw-One must render at least 2 waste cards (visible + buffer)" + ); + // Deduplicated length must equal pre-dedup length → all z distinct. + let raw_count = positions + .iter() + .filter(|(c, _, _)| waste_ids.contains(&c.id)) + .count(); + assert_eq!( + waste_zs.len(), + raw_count, + "all rendered waste card z values must be distinct" + ); + } }