From 6a1c7354c152564c33d8489f6ea07ac585da0651 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 21:19:37 -0400 Subject: [PATCH] security(anim): bound custom-show and timeline lookups, share motion paths across paragraph effects (#1341) * security(anim): index slides and shapes once, share paragraph targets and motion paths Custom-show resolution, timeline target lookup and by-paragraph builds scaled with the product of two document-controlled counts, and paragraph expansion deep-copied the parsed motion path into every effect. - show_order / custom_indices resolve slide ids through one id->index map. - Timeline::with indexes shapes once (same first-wins order as Slide::shape) for animation targets and base colours, and records each animated shape's colour with a set. - Paragraph targets are worked out once per shape and shared across its animations. - Effects share their EffectSpec (and so the motion path) by Rc instead of cloning it. Co-Authored-By: Claude Sonnet 5.5 * fix(anim): use Arc for EffectSpec to satisfy Sync and early return on empty animations --------- Co-authored-by: Claude Sonnet 5.5 --- crates/anim/src/show.rs | 28 +++++++++----- crates/anim/src/tests.rs | 50 ++++++++++++++++++++++++ crates/anim/src/timeline.rs | 77 +++++++++++++++++++++++++++---------- 3 files changed, 125 insertions(+), 30 deletions(-) diff --git a/crates/anim/src/show.rs b/crates/anim/src/show.rs index a1117c62..d2f4790e 100644 --- a/crates/anim/src/show.rs +++ b/crates/anim/src/show.rs @@ -1,10 +1,10 @@ //! Slide show navigation: click steps, slide order (hidden slides, custom shows, ranges), loop //! and automatic advance. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; -use deckcraft_model::Presentation; use deckcraft_model::text::{Action, Hyperlink}; +use deckcraft_model::{Presentation, SlideId}; use crate::Timeline; use crate::interact::SlideRun; @@ -83,12 +83,25 @@ pub struct ShowState { pub zoom: Option<(usize, usize)>, } +/// Index of each slide by id (the first slide wins a duplicate id), so resolving a custom show +/// costs the sum of its two lists, not their product. +fn slide_indices(pres: &Presentation) -> HashMap { + let mut map = HashMap::with_capacity(pres.slides.len()); + for (i, s) in pres.slides.iter().enumerate() { + map.entry(s.id).or_insert(i); + } + map +} + /// Slide indices the show visits in order: the custom show or range from Set Up Slide Show, minus /// hidden slides. pub fn show_order(pres: &Presentation) -> Vec { let n = pres.slides.len(); let base: Vec = match pres.show.custom_show.as_ref().and_then(|name| pres.custom_shows.iter().find(|c| &c.name == name)) { - Some(cs) => cs.slides.iter().filter_map(|id| pres.slides.iter().position(|s| s.id == *id)).collect(), + Some(cs) => { + let index = slide_indices(pres); + cs.slides.iter().filter_map(|id| index.get(id).copied()).collect() + } None => match pres.show.range { Some((a, b)) => { let a = (a.max(1) as usize).saturating_sub(1); @@ -104,12 +117,9 @@ pub fn show_order(pres: &Presentation) -> Vec { /// Visible slides of custom show `name`, in that show's order. fn custom_indices(pres: &Presentation, name: &str) -> Option> { let cs = pres.custom_shows.iter().find(|c| c.name == name)?; - let order: Vec = cs - .slides - .iter() - .filter_map(|id| pres.slides.iter().position(|s| s.id == *id)) - .filter(|&i| pres.slides.get(i).is_some_and(|s| !s.hidden)) - .collect(); + let index = slide_indices(pres); + let order: Vec = + cs.slides.iter().filter_map(|id| index.get(id).copied()).filter(|&i| pres.slides.get(i).is_some_and(|s| !s.hidden)).collect(); if order.is_empty() { None } else { Some(order) } } diff --git a/crates/anim/src/tests.rs b/crates/anim/src/tests.rs index b07afcef..a9ccb946 100644 --- a/crates/anim/src/tests.rs +++ b/crates/anim/src/tests.rs @@ -1082,3 +1082,53 @@ fn show_returns_to_the_zoom_slide() { s.goto(&p, 4); assert_eq!(s.next(&p), ShowAction::Slide(5)); } + +#[test] +fn custom_show_resolves_duplicate_ids_to_the_first_slide() { + let mut p = pres(3); + Arc::make_mut(&mut p.slides[2]).id = p.slides[0].id; + p.custom_shows = vec![CustomShow { name: "Dup".into(), slides: vec![p.slides[0].id, SlideId(1001), SlideId(424_242)] }]; + p.show.custom_show = Some("Dup".into()); + assert_eq!(show_order(&p), vec![0, 1]); +} + +#[test] +fn custom_show_with_many_entries_resolves_quickly() { + let mut p = pres(20_000); + let ids: Vec = p.slides.iter().map(|s| s.id).collect(); + p.custom_shows = vec![CustomShow { name: "All".into(), slides: ids.iter().rev().copied().collect() }]; + p.show.custom_show = Some("All".into()); + let t = std::time::Instant::now(); + let order = show_order(&p); + assert_eq!(order.len(), 20_000); + assert_eq!(order[0], 19_999); + assert!(t.elapsed().as_secs() < 5); +} + +#[test] +fn timeline_targets_the_first_shape_with_a_duplicate_id() { + let mut s = slide_with(vec![anim(7, AnimClass::Entrance, "fade", AnimStart::OnClick)]); + s.shapes[0] = shape(7, "first"); + s.shapes[1] = shape(7, "second"); + s.shapes[0].xfrm = Some(Xfrm::new(10.0, 10.0, 20.0, 20.0)); + let tl = Timeline::new(&s, W, H); + assert_eq!(tl.effects().len(), 1); + assert_eq!(tl.steps(), 1); +} + +#[test] +fn many_by_paragraph_animations_on_one_shape_share_paragraph_targets() { + let mut s = slide_with(vec![]); + s.shapes[0].text = Some(TextBody::from_text("one\ntwo\nthree")); + s.animations = (0..2_000) + .map(|_| { + let mut a = anim(1, AnimClass::Path, "customPath", AnimStart::OnClick); + a.text_build = TextBuild::ByParagraph; + a.path = Some("M 0 0 L 0.5 0.5 E".into()); + a + }) + .collect(); + let tl = Timeline::new(&s, W, H); + assert_eq!(tl.effects().len(), 6_000); + assert_eq!(tl.paragraph_targets(ShapeId(1)), vec![0, 1, 2]); +} diff --git a/crates/anim/src/timeline.rs b/crates/anim/src/timeline.rs index 54cd8d78..c995f2ea 100644 --- a/crates/anim/src/timeline.rs +++ b/crates/anim/src/timeline.rs @@ -1,10 +1,11 @@ //! A slide's animation sequence: click steps, effect start times and per-shape state over time. use std::collections::{HashMap, HashSet}; +use std::sync::Arc; use deckcraft_model::anim::{BuildRole, TextBuild, paragraph_build_roles}; use deckcraft_model::style::Fill; -use deckcraft_model::{AnimClass, AnimStart, Animation, ColorScheme, Rgba, SchemeSlot, ShapeId, Slide, Xfrm}; +use deckcraft_model::{AnimClass, AnimStart, Animation, ColorScheme, Rgba, SchemeSlot, Shape, ShapeId, Slide, Xfrm}; use crate::AnimState; use crate::effects::{EffectSpec, exit_base, motion_path}; @@ -85,7 +86,9 @@ enum After { #[derive(Clone, Debug)] struct Effect { timing: EffectTiming, - spec: EffectSpec, + /// Shared by every paragraph effect of one animation, so a large motion path is not copied + /// per paragraph. + spec: Arc, after: After, } @@ -162,6 +165,14 @@ impl Timeline { let accent = scheme.map(|s| s.get(SchemeSlot::Accent1)).unwrap_or(Rgba::rgb(0x15, 0x60, 0x82)); let mut tl = Timeline::default(); + if slide.animations.is_empty() { + return tl; + } + // Every shape by id, built once: resolving each animation's target by tree search would + // cost shapes x animations. + let shapes = shape_index(&slide.shapes); + // Paragraph targets of each shape, worked out for its first by-paragraph animation only. + let mut para_cache: HashMap>>> = HashMap::new(); // Group the animations by sequence in one pass, sequences in order of first appearance // (main first), so many distinct triggers cost linear work, not a scan per trigger. let mut seqs: Vec<(Sequence, Vec)> = vec![(Sequence::Main, Vec::new())]; @@ -190,32 +201,38 @@ impl Timeline { if tl.effects.len() >= MAX_EFFECTS { break; } - let shape = slide.shape(a.shape); + let shape = shapes.get(&a.shape).copied(); // Paragraph targets of this animation. - let paras: Vec> = match (a.paragraph, a.text_build) { - (Some(p), _) => vec![Some(p as usize)], + let paras: Arc>> = match (a.paragraph, a.text_build) { + (Some(p), _) => Arc::new(vec![Some(p as usize)]), (None, TextBuild::ByParagraph) => { - let mut v: Vec> = vec![]; - if let Some(t) = shape.and_then(|s| s.text.as_ref()) { - for (i, role) in paragraph_build_roles(t).into_iter().enumerate() { - match role { - BuildRole::Top => v.push(Some(i)), - BuildRole::Member(o) => { - if grouped.insert((a.shape, i, o)) { - tl.groups.push((a.shape, i, o)); + let cached = para_cache.entry(a.shape).or_insert_with(|| { + let mut v: Vec> = vec![]; + if let Some(t) = shape.and_then(|s| s.text.as_ref()) { + for (i, role) in paragraph_build_roles(t).into_iter().enumerate() { + match role { + BuildRole::Top => v.push(Some(i)), + BuildRole::Member(o) => { + if grouped.insert((a.shape, i, o)) { + tl.groups.push((a.shape, i, o)); + } } + BuildRole::Unowned => {} } - BuildRole::Unowned => {} } } - } - if v.is_empty() { vec![None] } else { v } + if v.is_empty() { + v.push(None) + } + Arc::new(v) + }); + Arc::clone(cached) } - _ => vec![None], + _ => Arc::new(vec![None]), }; let xf = xfrm_of(a.shape).or_else(|| shape.and_then(|s| s.xfrm)).unwrap_or(Xfrm::new(sw / 4.0, sh / 4.0, sw / 2.0, sh / 2.0)); let xf = sanitize_xfrm(xf, sw, sh); - let spec = build_spec(a, xf, sw, sh, &resolve); + let spec = Arc::new(build_spec(a, xf, sw, sh, &resolve)); let dur = secs(a.duration_ms); let delay = secs(a.delay_ms); let until_next_click = a.repeat == u32::MAX; @@ -284,7 +301,7 @@ impl Timeline { span, class: a.class, }, - spec: spec.clone(), + spec: Arc::clone(&spec), after: after.clone(), }); } @@ -295,12 +312,13 @@ impl Timeline { } } // Base colours of animated shapes. + let mut colored: HashSet = HashSet::new(); for e in &tl.effects { let id = e.timing.shape; - if tl.colors.iter().any(|(s, _)| *s == id) { + if !colored.insert(id) { continue; } - let c = match slide.shape(id).and_then(|s| s.fill.as_ref()) { + let c = match shapes.get(&id).copied().and_then(|s| s.fill.as_ref()) { Some(Fill::Solid { color }) => resolve(color), _ => accent, }; @@ -533,6 +551,23 @@ fn status(e: &EffectTiming, step: usize, t: f64) -> Status { Status::Active(p.clamp(0.0, 1.0)) } +/// Shapes by id in the order `Slide::shape` searches (a shape before its children, groups at most +/// 64 deep); the first shape with an id wins a duplicate. +fn shape_index(shapes: &[Shape]) -> HashMap { + fn walk<'a>(shapes: &'a [Shape], depth: usize, out: &mut HashMap) { + if depth > 64 { + return; + } + for s in shapes { + out.entry(s.id).or_insert(s); + walk(s.children(), depth + 1, out); + } + } + let mut out = HashMap::new(); + walk(shapes, 0, &mut out); + out +} + fn build_spec(a: &Animation, xf: Xfrm, sw: f64, sh: f64, resolve: &dyn Fn(&deckcraft_model::ColorRef) -> Rgba) -> EffectSpec { let effect = if a.class == AnimClass::Exit { exit_base(&a.effect).to_string() } else { a.effect.clone() }; let path = if a.class == AnimClass::Path { Some(motion_path(&a.effect, &a.option, a.path.as_deref())) } else { None };