Merge pull request #1363 from DWAA1660/fix/1237-comp-contains-cycles

Project: comp_contains walks precomps with a visited set, not recursion (#1237)

crates/engine/src/tests_limits.rs

@@ -102,3 +102,38 @@ fn commands_cap_sizes_at_30000_px() {
     let f = s.project.items.values().find_map(|i| if let ItemKind::Footage(f) = &i.kind { Some(f.clone()) } else { None }).unwrap();
     assert_eq!((f.width, f.height, f.frame_rate), (limits::MAX_SIZE, 8, FrameRate::FPS_29_97));
 }
+
+#[test]
+fn a_project_with_a_precomp_cycle_opens_adds_and_renders_without_running_out_of_stack() {
+    // Comps A and B nest each other (a damaged file); C is unrelated. Checking whether A
+    // contains C walked the A ⇄ B cycle until the stack ran out (#1237).
+    let mut s = Session::default();
+    let mut ids = vec![];
+    for name in ["A", "B", "C"] {
+        s.execute("comp.new", json!({"name": name, "width": 32, "height": 32, "frameRate": 30, "duration": 1})).unwrap();
+        ids.push(s.active_comp_id().unwrap());
+    }
+    let (a, b, c) = (ids[0], ids[1], ids[2]);
+    {
+        let proj = std::sync::Arc::make_mut(&mut s.project);
+        for (outer, inner) in [(a, b), (b, a)] {
+            let comp = proj.comp(outer).unwrap().clone();
+            let l = effectcraft_project::build::layer(proj, &comp, "pre", effectcraft_project::LayerSource::Comp { item: inner }, (32, 32), None);
+            let ItemKind::Comp(cc) = &mut proj.item_mut(outer).unwrap().kind else { panic!("comp") };
+            std::sync::Arc::make_mut(cc).layers.push(l);
+        }
+    }
+    let path = tmp("cycle.ecproj");
+    std::fs::write(&path, s.project.to_file_json().unwrap()).unwrap();
+    let mut t = Session::default();
+    t.execute("file.open", json!({"path": path})).unwrap();
+    t.open_comp(c);
+    // Adding A into C asks whether A contains C: it doesn't, so the layer goes in.
+    let r = t.execute("layer.addItem", json!({"comp": c.0, "item": a.0})).unwrap();
+    assert!(r["layer"].as_u64().is_some(), "{r}");
+    // Adding C into A would nest C in itself through A: refused, as before.
+    assert!(t.execute("layer.addItem", json!({"comp": a.0, "item": c.0})).is_err());
+    // C renders: inside A the renderer skips B's copy of A instead of looping.
+    let img = t.render(c, Tick::ZERO, RenderOpts { scale: 1.0, ..Default::default() });
+    assert_eq!((img.width, img.height), (32, 32));
+}

crates/project/src/lib.rs

@@ -1478,11 +1478,32 @@ impl Project {
     }
     /// Whether comp `inner` is (transitively) nested inside comp `outer` (to prevent cycles).
     pub fn comp_contains(&self, outer: ItemId, inner: ItemId) -> bool {
+        // A walk with a stack and a visited set rather than recursion (#1237): a damaged project
+        // may hold a precomp cycle that doesn't pass `inner` (recursion never ended), a long
+        // chain must not use up the call stack, and comps shared by many paths are visited once
+        // (recursion re-walked them for every path).
         if outer == inner {
             return true;
         }
-        let Some(c) = self.comp(outer) else { return false };
+        let mut seen = std::collections::HashSet::new();
-        c.layers.iter().any(|l| matches!(l.source, LayerSource::Comp { item } if self.comp_contains(item, inner)))
+        let mut stack = vec![outer];
+        while let Some(id) = stack.pop() {
+            if !seen.insert(id) {
+                continue;
+            }
+            let Some(c) = self.comp(id) else { continue };
+            for l in &c.layers {
+                if let LayerSource::Comp { item } = l.source {
+                    if item == inner {
+                        return true;
+                    }
+                    if !seen.contains(&item) {
+                        stack.push(item);
+                    }
+                }
+            }
+        }
+        false
     }
     /// Make sure `next_id` exceeds every id in the project (after loading or merging).
     pub fn fix_next_id(&mut self) {

crates/project/src/tests.rs

@@ -212,3 +212,58 @@ fn item_users_count_layers_in_every_comp() {
     assert!(p.item_users(unused).is_empty());
     assert!(p.item_users(main).is_empty());
 }
+
+/// A project of `n` empty comps; `links` adds a precomp layer of comp `b` inside comp `a`
+/// (indexes into the returned ids).
+fn comp_graph(n: usize, links: &[(usize, usize)]) -> (Project, Vec<crate::ItemId>) {
+    let mut p = Project::default();
+    let ids: Vec<crate::ItemId> = (0..n)
+        .map(|i| {
+            p.add_item(
+                &format!("C{i}"),
+                effectcraft_color::Label::None,
+                None,
+                ItemKind::Comp(Comp::new(8, 8, FrameRate::FPS_25, Tick::from_seconds_f64(1.0)).into()),
+            )
+        })
+        .collect();
+    for &(a, b) in links {
+        let outer = p.comp(ids[a]).unwrap().clone();
+        let l = build::layer(&mut p, &outer, "pre", LayerSource::Comp { item: ids[b] }, (8, 8), None);
+        let ItemKind::Comp(c) = &mut p.item_mut(ids[a]).unwrap().kind else { panic!("comp") };
+        std::sync::Arc::make_mut(c).layers.push(l);
+    }
+    (p, ids)
+}
+
+#[test]
+fn comp_contains_ends_on_cycles_long_chains_and_shared_precomps() {
+    // A ⇄ B cycle, C unrelated: the walk used to recurse until the stack ran out (#1237).
+    let (p, c) = comp_graph(3, &[(0, 1), (1, 0)]);
+    assert!(!p.comp_contains(c[0], c[2]));
+    assert!(p.comp_contains(c[0], c[1]) && p.comp_contains(c[1], c[0]));
+    // A comp nesting itself.
+    let (p, c) = comp_graph(2, &[(0, 0)]);
+    assert!(!p.comp_contains(c[0], c[1]));
+    assert!(p.comp_contains(c[0], c[0]));
+    // A target reached through a cycle.
+    let (p, c) = comp_graph(3, &[(0, 1), (1, 0), (1, 2)]);
+    assert!(p.comp_contains(c[0], c[2]));
+    assert!(!p.comp_contains(c[2], c[0]));
+    // 40 levels of diamonds: 2^40 paths, each comp visited once.
+    let mut links = vec![];
+    for i in 0..40 {
+        let (top, l, r, bottom) = (i * 3, i * 3 + 1, i * 3 + 2, i * 3 + 3);
+        links.extend([(top, l), (top, r), (l, bottom), (r, bottom)]);
+    }
+    let (p, c) = comp_graph(121, &links);
+    let t0 = std::time::Instant::now();
+    assert!(p.comp_contains(c[0], c[120]));
+    assert!(!p.comp_contains(c[120], c[0]));
+    assert!(t0.elapsed() < std::time::Duration::from_secs(1), "{:?}", t0.elapsed());
+    // A chain 20 000 comps deep (a deep recursion's stack frames).
+    let n = 20_000;
+    let (p, c) = comp_graph(n, &(0..n - 1).map(|i| (i, i + 1)).collect::<Vec<_>>());
+    assert!(p.comp_contains(c[0], c[n - 1]));
+    assert!(!p.comp_contains(c[n - 1], c[0]));
+}