From 1a26bb0b2ae3949c944c58fb96e044267a4dc3f3 Mon Sep 17 00:00:00 2001 From: grom Date: Thu, 15 Oct 2026 20:46:40 -0600 Subject: [PATCH] portrait: one stuck pane stops starving the rest of its page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_pane_previews` assembles one pane a frame and chose the first stale one. A pane that cannot be assembled — an item model that never resolves — is stale by definition, so it was chosen again every frame and gave the frame up every time: every pane behind it in slot order waited on it forever and stayed on the mirrored live player, which looks exactly like the per-pane dress-up never having run. The shop's Smoky Wooden Torch was doing this to its page. The choice now offers every stale pane in slot order and the caller takes the first it can actually make, so a pane that cannot be assembled costs its own turn and nothing else. Walking past one is free: every `assemble` bail is a lookup, all of them before the skin composite. Also adds `[booth] pane source`, a `WOW_BOOTH_LOG` line per pooled slot whenever where its character comes from changes — its own bake, or the mirror because the room is closed, the bake has no parts, or its rig is still loading. Those three look identical on screen and were the reason this took a second run to see. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/entities/attach/preview.rs | 182 +++++++++++------- crates/benilla-app/src/portrait/mod.rs | 31 +++ 2 files changed, 146 insertions(+), 67 deletions(-) diff --git a/crates/benilla-app/src/entities/attach/preview.rs b/crates/benilla-app/src/entities/attach/preview.rs index dfa181f2..b5b88ba9 100644 --- a/crates/benilla-app/src/entities/attach/preview.rs +++ b/crates/benilla-app/src/entities/attach/preview.rs @@ -417,87 +417,106 @@ pub(in crate::entities) fn build_pane_previews( .by_pane .retain(|pane, _| preview.panes.contains_key(pane)); - let Some((pane, look)) = next_stale_pane(pool.pooled_panes(), &preview.panes, &bakes.by_pane) - else { - return; - }; let Some(creatures) = creatures.as_deref() else { return; }; - let spec = pane_spec(&look); - let Some(a) = assemble( - &spec, - &mut PreviewCtx { - creatures, - characters: characters.as_deref(), - displays: displays.as_deref_mut(), - glows: glows.as_deref_mut(), - sections: sections.as_deref(), - world_assets: world_assets.as_deref(), - images: &mut images, - skin_composites: &mut skin_composites, - asset_server: &asset_server, - mats: &mut mats, - }, - ) else { - return; // an item model is still loading: retry next frame + // Every stale pane this frame may try, in slot order, collected because the assembly below + // needs `bakes` mutably to write its result. + let stale: Vec<_> = stale_panes(pool.pooled_panes(), &preview.panes, &bakes.by_pane).collect(); + let mut ctx = PreviewCtx { + creatures, + characters: characters.as_deref(), + displays: displays.as_deref_mut(), + glows: glows.as_deref_mut(), + sections: sections.as_deref(), + world_assets: world_assets.as_deref(), + images: &mut images, + skin_composites: &mut skin_composites, + asset_server: &asset_server, + mats: &mut mats, }; - let revision = bakes - .by_pane - .get(&pane) - .map_or(0, |bake| bake.revision) - .wrapping_add(1); - // The equipment fingerprint, not just the counts: two panes wearing different boots assemble - // the same number of parts, so counts cannot tell "every pane composed its own look" from - // "every pane got one pane's look" — which is exactly the question a page of identical-looking - // previews asks. - let worn: Vec = look - .equipment - .iter() - .enumerate() - .filter(|(_, item)| item.display_id != 0) - .map(|(slot, item)| format!("{slot}:{}", item.display_id)) - .collect(); - debug!( - "pane preview: pane {pane:?} race {} display {} → {} parts, {} riders, {} effect model(s); \ - worn [{}]", - look.race, - look.display_id, - a.parts.len(), - a.riders.len(), - a.effects.len(), - worn.join(" "), - ); - bakes.by_pane.insert( - pane, - DressUpBake { - look: Some(look), - display_id: look.display_id, - parts: a.parts, - riders: a.riders, - effects: a.effects, - billboards: a.billboards, - grip: a.grip, - revision, - }, - ); + // One assembly a frame, but the *first that can be made*, not the first that is stale. + // + // A pane whose assembly cannot complete is chosen again every frame — it stays stale by + // definition — so giving the frame up on it starved every pane behind it in slot order for as + // long as its model refused to resolve. One item doing that (the shop's Smoky Wooden Torch) + // left the rest of the page on the mirrored live player, which looks exactly like the + // per-pane dress-up having failed. Walking past it costs nothing: every `assemble` bail is a + // lookup before the skin composite, the expensive half. + for (pane, look) in stale { + let spec = pane_spec(&look); + let Some(a) = assemble(&spec, &mut ctx) else { + continue; // an item model is still loading: the pane behind it can still have its turn + }; + let revision = bakes + .by_pane + .get(&pane) + .map_or(0, |bake| bake.revision) + .wrapping_add(1); + // The equipment fingerprint, not just the counts: two panes wearing different boots + // assemble the same number of parts, so counts cannot tell "every pane composed its own + // look" from "every pane got one pane's look" — which is exactly the question a page of + // identical-looking previews asks. + let worn: Vec = look + .equipment + .iter() + .enumerate() + .filter(|(_, item)| item.display_id != 0) + .map(|(slot, item)| format!("{slot}:{}", item.display_id)) + .collect(); + debug!( + "pane preview: pane {pane:?} race {} display {} → {} parts, {} riders, {} effect \ + model(s); worn [{}]", + look.race, + look.display_id, + a.parts.len(), + a.riders.len(), + a.effects.len(), + worn.join(" "), + ); + bakes.by_pane.insert( + pane, + DressUpBake { + look: Some(look), + display_id: look.display_id, + parts: a.parts, + riders: a.riders, + effects: a.effects, + billboards: a.billboards, + grip: a.grip, + revision, + }, + ); + return; + } } -/// The first pooled pane whose assembled bake is missing or no longer matches its look, in slot -/// order; `None` when every drawn pane is current. +/// Every pooled pane whose assembled bake is missing or no longer matches its look, in slot +/// order; empty when every drawn pane is current. /// /// Pulled out of the system so the choice is testable without a world: "one pane per frame" is /// only safe if the choice also *advances*, and the failure mode if it does not — one pane /// re-assembled forever while the rest of the page stays blank — looks identical to an item model -/// that will not load. +/// that will not load. The caller walks this in order and stops at the first pane it can actually +/// assemble, so a pane that is permanently unassemblable costs its own turn and nothing else. +fn stale_panes<'a>( + pooled: impl Iterator + 'a, + looks: &'a std::collections::HashMap, + bakes: &'a std::collections::HashMap, +) -> impl Iterator + 'a { + pooled + .filter_map(|pane| looks.get(&pane).map(|look| (pane, *look))) + .filter(|(pane, look)| bakes.get(pane).is_none_or(|bake| bake.look != Some(*look))) +} + +/// The first of [`stale_panes`]; the shape the choice tests ask about. +#[cfg(test)] fn next_stale_pane( pooled: impl Iterator, looks: &std::collections::HashMap, bakes: &std::collections::HashMap, ) -> Option<(benilla_ui::widget::FrameHandle, DressUpLook)> { - pooled - .filter_map(|pane| looks.get(&pane).map(|look| (pane, *look))) - .find(|(pane, look)| bakes.get(pane).is_none_or(|bake| bake.look != Some(*look))) + stale_panes(pooled, looks, bakes).next() } /// A pane's look as an assembly spec. The flag choices are [`build_dressup_preview`]'s, for the @@ -1401,7 +1420,7 @@ mod pane_preview_tests { use benilla_ui::widget::{FrameKind, WidgetArena}; - use super::{next_stale_pane, DressUpBake, DressUpLook}; + use super::{next_stale_pane, stale_panes, DressUpBake, DressUpLook}; /// Two real handles from a real arena: `FrameHandle` is generational and has no public /// constructor, which is the right call — a handle the arena never minted could not resolve. @@ -1469,6 +1488,35 @@ mod pane_preview_tests { ); } + /// Every stale pane is offered, in slot order — not just the first. + /// + /// The caller assembles one pane a frame and walks this list until one of them can actually + /// be made. A pane whose item model never resolves is stale forever and is therefore offered + /// first forever, so if the list stopped at it, every pane behind it would wait on it: the + /// shop's Smoky Wooden Torch kept a whole page of cards on the mirrored live player that way. + #[test] + fn a_pane_that_cannot_assemble_does_not_hide_the_ones_behind_it() { + let mut arena = WidgetArena::default(); + let stuck = arena.create(FrameKind::Frame, None, None); + let (b, c) = ( + arena.create(FrameKind::Frame, None, None), + arena.create(FrameKind::Frame, None, None), + ); + let looks: HashMap<_, _> = [(stuck, look(10)), (b, look(20)), (c, look(30))] + .into_iter() + .collect(); + let bakes = HashMap::new(); + + let offered: Vec<_> = stale_panes([stuck, b, c].into_iter(), &looks, &bakes) + .map(|(pane, _)| pane) + .collect(); + assert_eq!( + offered, + vec![stuck, b, c], + "the pane that cannot be assembled comes first and the rest follow it" + ); + } + /// Nothing to do once every drawn pane matches its look — the steady state a settled shop /// page sits in, where this system must cost nothing. #[test] diff --git a/crates/benilla-app/src/portrait/mod.rs b/crates/benilla-app/src/portrait/mod.rs index ad8f258c..c7ae8d39 100644 --- a/crates/benilla-app/src/portrait/mod.rs +++ b/crates/benilla-app/src/portrait/mod.rs @@ -2214,6 +2214,9 @@ fn sync_body_booths( mut cams: Query<(&BoothCam, &mut Transform, &mut Projection)>, framing_in: BoothFraming, anim_data: Option>, + // The last reason each pooled slot drew what it drew, so `WOW_BOOTH_LOG` prints one line per + // change instead of one a frame. + mut pane_source: Local>, ) { if test_mode(&mut env_cache) { return; // the test bake owns the booths @@ -2270,6 +2273,18 @@ fn sync_body_booths( // path below can only ever show what the player is actually wearing, which is why a page of // cards used to be a page of the same character and the Fashion tab could not show an item // on you. When `build_pane_previews` has assembled this pane's look, stage that instead. + // Why this slot draws what it draws, logged under `WOW_BOOTH_LOG` and only when it + // changes. A pooled pane that falls through to the mirror shows the live player, and on a + // page of cards that is indistinguishable by eye from the per-pane dress-up never having + // run at all — so the one thing the log has to say is *which* of the three it is. + let mut why = match claim { + None => "idle: no pane claims this slot", + Some(c) => match subjects.bakes.by_pane.get(&c.pane) { + None => "mirror: no bake (its room is closed, or it has not assembled yet)", + Some(bake) if bake.parts.is_empty() => "mirror: the bake assembled to no parts", + Some(_) => "staged: its own look", + }, + }; if let Some((pane, staged)) = claim .map(|c| c.pane) .and_then(|pane| subjects.bakes.by_pane.get(&pane).map(|bake| (pane, bake))) @@ -2293,11 +2308,14 @@ fn sync_body_booths( &framing_in.panes, last_pose.entry(slot).or_default(), ) { + log_pane_source(slot, claim.map(|c| c.pane), why, &mut pane_source); continue; } // Not ready (a material or the rig is still loading): fall through to the mirror for // this frame rather than leaving the pane blank, and restage when it lands. + why = "mirror: the bake's rig or a material is still loading"; } + log_pane_source(slot, claim.map(|c| c.pane), why, &mut pane_source); sync_body_booth( &mut palettes, slot, @@ -2322,6 +2340,19 @@ fn sync_body_booths( } } +/// `WOW_BOOTH_LOG=1`: one line whenever a pooled slot changes where its character comes from. +fn log_pane_source( + slot: &'static str, + pane: Option, + why: &'static str, + seen: &mut HashMap<&'static str, &'static str>, +) { + if !booth_log() || seen.insert(slot, why) == Some(why) { + return; + } + eprintln!("[booth] pane source slot={slot} pane={pane:?} {why}"); +} + /// Does the booth have to be rebuilt for this pane's bake? /// /// Keyed on the **pane and** the revision, never the revision alone: every pane numbers its own