diff --git a/crates/engine/src/tests/disk_cache.rs b/crates/engine/src/tests/disk_cache.rs index 119699aa8..41497dd4b 100644 --- a/crates/engine/src/tests/disk_cache.rs +++ b/crates/engine/src/tests/disk_cache.rs @@ -136,3 +136,62 @@ fn footage_changes_change_the_salt() { assert_ne!(k3, k2); assert_ne!(set_proxy(&mut s, false), k3); } + +/// Marker edits keep the disk key (the sound of a playing preview must not wait for frames to +/// render again, #1083); an expression that reads markers makes them count. +#[test] +fn markers_keep_the_content_key_unless_an_expression_reads_them() { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "M", "width": 8, "height": 8, "frameRate": 30, "duration": 2})).unwrap(); + let cid = s.active_comp_id().unwrap(); + let l = s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap()["layer"].as_u64().unwrap(); + let key = |s: &Session| effectcraft_render::disk_cache::comp_content_key(&s.project, cid); + let k0 = key(&s); + s.execute("comp.addMarker", json!({"time": 0.5, "comment": "beat"})).unwrap(); + s.execute("markers.set", json!({"layer": l, "new": true, "time": 0.25})).unwrap(); + assert_eq!(key(&s), k0, "markers"); + s.execute("prop.setExpression", json!({"layer": l, "path": "transform/opacity", "expression": "thisComp.marker.numKeys"})).unwrap(); + let k1 = key(&s); + assert_ne!(k1, k0); + s.execute("comp.addMarker", json!({"time": 1.0})).unwrap(); + assert_ne!(key(&s), k1, "a marker read by an expression"); +} + +/// A comp marker alone (no layer marker beside it) keeps the key too. +#[test] +fn a_lone_comp_marker_keeps_the_content_key() { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "M", "width": 8, "height": 8, "frameRate": 30, "duration": 2})).unwrap(); + let cid = s.active_comp_id().unwrap(); + s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap(); + let key = |s: &Session| effectcraft_render::disk_cache::comp_content_key(&s.project, cid); + let k0 = key(&s); + s.execute("comp.addMarker", json!({"time": 0.5, "comment": "beat"})).unwrap(); + assert_eq!(key(&s), k0); +} + +/// A protected marker with a duration retimes a stretched precomp layer (Responsive Design), so +/// changing it changes the key of the comp that nests it; a plain marker beside it does not. +#[test] +fn protected_markers_with_a_duration_change_the_content_key() { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Inner", "width": 8, "height": 8, "frameRate": 30, "duration": 4})).unwrap(); + let inner = s.active_comp_id().unwrap(); + s.execute("layer.newSolid", json!({"color": "#ff0000"})).unwrap(); + s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "frameRate": 30, "duration": 10})).unwrap(); + let outer = s.active_comp_id().unwrap(); + let l = s.execute("layer.addItem", json!({"item": inner.0})).unwrap()["layer"].as_u64().unwrap(); + s.execute("layer.select", json!({"layers": [l]})).unwrap(); + s.execute("layer.timeStretch", json!({"percent": 200})).unwrap(); + let key = |s: &Session| effectcraft_render::disk_cache::comp_content_key(&s.project, outer); + let k0 = key(&s); + s.execute("comp.addMarker", json!({"comp": inner.0, "time": 2.0, "comment": "beat"})).unwrap(); + assert_eq!(key(&s), k0, "a plain marker"); + s.execute("comp.addMarker", json!({"comp": inner.0, "time": 1.0})).unwrap(); + let index = s.project.comp(inner).unwrap().markers.iter().position(|m| m.time.seconds() == 1.0).unwrap(); + s.execute("markers.set", json!({"comp": inner.0, "index": index, "duration": 1.0, "protected": true})).unwrap(); + let k1 = key(&s); + assert_ne!(k1, k0, "a protected marker with a duration"); + s.execute("markers.set", json!({"comp": inner.0, "index": index, "duration": 2.0})).unwrap(); + assert_ne!(key(&s), k1, "its duration"); +} diff --git a/crates/project/src/comp.rs b/crates/project/src/comp.rs index 5a3c8650e..36069e637 100644 --- a/crates/project/src/comp.rs +++ b/crates/project/src/comp.rs @@ -275,15 +275,35 @@ impl Comp { } } +/// A protected time marker with a duration sets the comp's timing (Responsive Design), so it is +/// part of the pixels even where plain markers are not. +fn is_timing_marker(m: &Marker) -> bool { + m.protected && m.duration > Tick::ZERO +} + +fn timing_markers(m: &[Marker]) -> impl Iterator + '_ { + m.iter().filter(|m| is_timing_marker(m)).map(|m| (m.time, m.duration)) +} + impl Comp { /// Draws the same pixels as `o`: equal but for its layers' Audio, Lock and Shy switches, Hide /// Shy Layers and the viewer's Resolution. pub fn same_pixels(&self, o: &Comp) -> bool { - self.same_settings(o) && self.layers.len() == o.layers.len() && self.layers.iter().zip(&o.layers).all(|(a, b)| a.same_pixels(b)) + self.same_pixels_with(o, true) } - /// [`Self::same_pixels`] but for the layers. - fn same_settings(&self, o: &Comp) -> bool { + /// [`Self::same_pixels`], with the comp's and its layers' markers counting only when + /// `with_markers`: only expressions read them, so while none does, adding or moving a marker + /// (even during playback) leaves every rendered frame valid. Protected time markers with a + /// duration set the timing (Responsive Design), so those always count. + pub fn same_pixels_with(&self, o: &Comp, with_markers: bool) -> bool { + self.same_settings(o, with_markers) + && self.layers.len() == o.layers.len() + && self.layers.iter().zip(&o.layers).all(|(a, b)| a.same_pixels_with(b, with_markers)) + } + + /// [`Self::same_pixels_with`] but for the layers. + fn same_settings(&self, o: &Comp, with_markers: bool) -> bool { // Every field (no `..`: a new field must be considered here). let Comp { width, @@ -321,7 +341,7 @@ impl Comp { && *display_start == o.display_start && *background == o.background && *work_area == o.work_area - && *markers == o.markers + && (if with_markers { *markers == o.markers } else { timing_markers(markers).eq(timing_markers(&o.markers)) }) && *shutter_angle == o.shutter_angle && *shutter_phase == o.shutter_phase && *motion_blur_samples == o.motion_blur_samples @@ -356,7 +376,7 @@ impl Comp { }) } // Solo hides every other layer at every frame. - if !self.same_settings(o) || self.any_solo() != o.any_solo() { + if !self.same_settings(o, true) || self.any_solo() != o.any_solo() { return None; } let common = |a: &Comp, b: &Comp| a.layers.iter().map(|l| l.id).filter(|id| b.layer(*id).is_some()).collect::>(); @@ -393,34 +413,33 @@ impl Comp { self.layers.iter().any(|l| l.switches.solo && l.source.is_av()) } - /// Draws the same pixels as `o` once plain markers are ignored: they change no pixels. Protected - /// time markers with a duration set the timing (Responsive Design), so those still count. - pub fn same_pixels_ignoring_markers(&self, o: &Comp) -> bool { - let protected = - |m: &[Marker]| -> Vec<(Tick, Tick)> { m.iter().filter(|m| m.protected && m.duration > Tick::ZERO).map(|m| (m.time, m.duration)).collect() }; - if protected(&self.markers) != protected(&o.markers) { - return false; - } - let mut c = self.clone(); - c.markers = o.markers.clone(); - for (a, b) in c.layers.iter_mut().zip(&o.layers) { - a.markers = b.markers.clone(); - a.markers_locked = b.markers_locked; - } - c.same_pixels(o) - } - /// The comp with those switches and the Resolution at their defaults (borrowed when they /// already are), for keys that hash the whole comp. pub fn pixel_form(&self) -> std::borrow::Cow<'_, Comp> { - if !self.hide_shy && self.resolution.is_auto() && self.layers.iter().all(|l| l.switches == l.switches.pixels()) { + self.pixel_form_with(true) + } + + /// [`Self::pixel_form`], also without the comp's plain markers and its layers' markers when not + /// `with_markers` (see [`Self::same_pixels_with`] and [`Project::reads_markers`]). + pub fn pixel_form_with(&self, with_markers: bool) -> std::borrow::Cow<'_, Comp> { + if !self.hide_shy + && self.resolution.is_auto() + && self.layers.iter().all(|l| l.switches == l.switches.pixels()) + && (with_markers || (self.markers.iter().all(is_timing_marker) && self.layers.iter().all(|l| l.markers.is_empty()))) + { return std::borrow::Cow::Borrowed(self); } let mut c = self.clone(); c.hide_shy = false; c.resolution = Resolution::Auto; + if !with_markers { + c.markers.retain(is_timing_marker); + } for l in &mut c.layers { l.switches = l.switches.pixels(); + if !with_markers { + l.markers.clear(); + } } std::borrow::Cow::Owned(c) } diff --git a/crates/project/src/item.rs b/crates/project/src/item.rs index 3f53bc823..22759e27d 100644 --- a/crates/project/src/item.rs +++ b/crates/project/src/item.rs @@ -98,6 +98,21 @@ impl Default for Project { } impl Project { + /// Some layer has an expression, which can read markers (`marker.key(1)`, `numKeys`, + /// `nearestKey`...). Only then do the markers of comps and layers affect what is rendered, so + /// frame caches ignore them otherwise. + pub fn reads_markers(&self) -> bool { + // Called for every frame identity and disk-cache key: stops at the first expression and + // builds no paths (`PropGroup::walk` formats one per node). + fn any_expr(g: &crate::PropGroup) -> bool { + g.children.iter().any(|c| match c { + crate::Node::Prop(p) => p.expr.is_some(), + crate::Node::Group(sub) => any_expr(sub), + }) + } + self.comps().any(|(_, c)| c.layers.iter().any(|l| any_expr(&l.props))) + } + /// Allocate an id (items, layers and property uids share one counter). pub fn alloc(&mut self) -> u64 { let id = self.next_id; diff --git a/crates/project/src/layer.rs b/crates/project/src/layer.rs index 25e2da7c4..21307583d 100644 --- a/crates/project/src/layer.rs +++ b/crates/project/src/layer.rs @@ -195,6 +195,12 @@ impl Switches { impl Layer { /// Draws the same pixels as `o`: equal but for the Audio, Lock and Shy switches. pub fn same_pixels(&self, o: &Layer) -> bool { + self.same_pixels_with(o, true) + } + + /// [`Self::same_pixels`], with the markers counting only when `with_markers` (they are read by + /// expressions alone, so a project without any can edit them freely). + pub fn same_pixels_with(&self, o: &Layer, with_markers: bool) -> bool { // Every field (no `..`: a new field must be considered here); the property tree last. let Layer { id, @@ -232,7 +238,7 @@ impl Layer { && *preserve_transparency == o.preserve_transparency && *track_matte == o.track_matte && *parent == o.parent - && *markers == o.markers + && (!with_markers || *markers == o.markers) && *markers_locked == o.markers_locked && *auto_orient == o.auto_orient && *environment == o.environment diff --git a/crates/render/src/disk_cache.rs b/crates/render/src/disk_cache.rs index 1919bc5c6..8d6761516 100644 --- a/crates/render/src/disk_cache.rs +++ b/crates/render/src/disk_cache.rs @@ -903,7 +903,8 @@ pub fn footage_salt_with(project: &effectcraft_project::Project, virtual_id: Opt /// Content key of a composition's pixels: project render settings, the comp and everything it /// uses (nested comps, footage, solids, their proxies and Use Proxy switches, footage and proxy /// files' size and time). With expressions in play (which can read any comp) the whole project -/// counts. Switches that don't change pixels (Audio, Lock, Shy, Hide Shy Layers) don't count. +/// counts. Switches that don't change pixels (Audio, Lock, Shy, Hide Shy Layers) don't count, and +/// neither do markers unless an expression mentions them. pub fn comp_content_key(project: &effectcraft_project::Project, comp: effectcraft_project::ItemId) -> u128 { comp_content_key_with(project, comp, None) } @@ -942,13 +943,15 @@ fn comp_content_key_for_renderer( } } } + // Markers are pixels only to the expressions that read them (#1083). + let markers = project.reads_markers(); let ids: Vec = if expressions { project.items.keys().copied().collect() } else { seen.into_iter().collect() }; for id in ids { let Some(it) = project.item(id) else { continue }; h.write_u64(id.0); h.write(it.name.as_bytes()); match &it.kind { - ItemKind::Comp(c) => hash_debug(&mut h, &c.pixel_form()), + ItemKind::Comp(c) => hash_debug(&mut h, &c.pixel_form_with(markers)), k => hash_debug(&mut h, k), } if let ItemKind::Footage(f) = &it.kind { diff --git a/crates/ui-egui/src/frames.rs b/crates/ui-egui/src/frames.rs index fcbb19e7b..b524b5959 100644 --- a/crates/ui-egui/src/frames.rs +++ b/crates/ui-egui/src/frames.rs @@ -317,23 +317,6 @@ pub fn comp_content(project: &Project, comp: ItemId) -> u64 { comp_content_with(project, comp, project, |_, c| c) } -/// Whether two versions of a comp draw the same pixels. Markers only count when an expression -/// could read them (`markers_count`). -fn same_pixels(a: &Comp, b: &Comp, markers_count: bool) -> bool { - if markers_count { a.same_pixels(b) } else { a.same_pixels_ignoring_markers(b) } -} - -/// Whether a layer anywhere in the project has an expression, which can read markers. -fn project_has_expressions(project: &Project) -> bool { - fn has_expression(g: &PropGroup) -> bool { - g.children.iter().any(|c| match c { - Node::Prop(p) => p.expr.is_some(), - Node::Group(sub) => has_expression(sub), - }) - } - project.items.values().any(|i| matches!(&i.kind, ItemKind::Comp(c) if c.layers.iter().any(|l| has_expression(&l.props)))) -} - /// [`comp_content`] with each comp's version selected for both its sources and its address. /// Sources removed by undo are read from `base`. fn comp_content_with<'a>(project: &'a Project, comp: ItemId, base: &'a Project, version: impl Fn(ItemId, &'a Arc) -> &'a Arc) -> u64 { @@ -762,12 +745,12 @@ impl Frames { fn identity(&self, project: &Arc, comp: ItemId) -> Identity { let plain = comp_content(project, comp); let Ok(mut bases) = self.bases.lock() else { return (plain, None) }; - // Markers can only change pixels through expressions, which read them. - let markers_count = project_has_expressions(project); + // Markers are pixels only to the expressions that read them (#1083). + let markers = project.reads_markers(); if let Some((base_id, base)) = bases.get(&comp.0) && *base_id != plain && comp_content_with(project, comp, project, |id, c| match base.item(id).map(|i| &i.kind) { - Some(ItemKind::Comp(b)) if Arc::ptr_eq(b, c) || same_pixels(b, c, markers_count) => b, + Some(ItemKind::Comp(b)) if Arc::ptr_eq(b, c) || b.same_pixels_with(c, markers) => b, _ => c, }) == *base_id { @@ -779,7 +762,7 @@ impl Frames { if let Some((base_id, base)) = bases.get(&comp.0) && let (Some(old), Some(new)) = (base.comp(comp), project.comp(comp)) && comp_content_with(project, comp, base, |id, c| match base.item(id).map(|i| &i.kind) { - Some(ItemKind::Comp(b)) if id == comp || Arc::ptr_eq(b, c) || same_pixels(b, c, markers_count) => b, + Some(ItemKind::Comp(b)) if id == comp || Arc::ptr_eq(b, c) || b.same_pixels_with(c, markers) => b, _ => c, }) == *base_id && let Some((s, e)) = new.changed_span(old) diff --git a/crates/ui-egui/src/frames/tests.rs b/crates/ui-egui/src/frames/tests.rs index 6da1dc269..a2603dfec 100644 --- a/crates/ui-egui/src/frames/tests.rs +++ b/crates/ui-egui/src/frames/tests.rs @@ -895,33 +895,59 @@ fn adding_a_source_does_not_hide_changes_to_existing_dependencies() { } } +/// Adding, moving or deleting a marker (Num* while a preview plays) keeps the cached frames: +/// the sound would otherwise stop at the next frame that has to be rendered again (#1083). +/// Only expressions that read markers make them part of the pixels. #[test] -fn markers_keep_the_frames() { +fn markers_keep_the_frames_unless_an_expression_reads_them() { use effectcraft_engine::Session; use serde_json::json; let mut s = Session::default(); let id = |v: serde_json::Value, k: &str| v[k].as_u64().unwrap(); - let outer = ItemId(id(s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "duration": 1})).unwrap(), "comp")); - let solid = id(s.execute("layer.newSolid", json!({"comp": outer.0, "color": "#ff0000"})).unwrap(), "layer"); + let inner = ItemId(id(s.execute("comp.new", json!({"name": "Inner", "width": 8, "height": 8, "duration": 2})).unwrap(), "comp")); + let solid = id(s.execute("layer.newSolid", json!({"comp": inner.0, "color": "#ff0000"})).unwrap(), "layer"); + let outer = ItemId(id(s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "duration": 2})).unwrap(), "comp")); + id(s.execute("layer.addItem", json!({"comp": outer.0, "item": inner.0})).unwrap(), "layer"); let f = Frames::default(); - let at = |s: &Session| f.content_of(&s.project, s.revision, outer); - let o0 = at(&s); - s.execute("layer.addMarker", json!({"comp": outer.0, "layers": [solid], "time": 0.5, "comment": "beat"})).unwrap(); - assert_eq!(at(&s), o0, "a marker changes no pixels, so it keeps the frames"); + let at = |s: &Session, c: ItemId| f.content_of(&s.project, s.revision, c); + let (o0, i0) = (at(&s, outer), at(&s, inner)); + s.execute("comp.addMarker", json!({"comp": inner.0, "time": 0.5, "comment": "beat"})).unwrap(); + s.execute("comp.addMarker", json!({"comp": outer.0, "time": 1.0})).unwrap(); + s.execute("comp.addMarker", json!({"comp": outer.0, "time": 1.5})).unwrap(); + assert_eq!((at(&s, outer), at(&s, inner)), (o0, i0), "comp markers"); + s.execute("markers.set", json!({"comp": inner.0, "layer": solid, "new": true, "time": 0.25})).unwrap(); + assert_eq!((at(&s, outer), at(&s, inner)), (o0, i0), "layer marker"); + // An expression reading markers makes them count again. + s.execute("prop.setExpression", json!({"comp": inner.0, "layer": solid, "path": "transform/opacity", "expression": "thisComp.marker.numKeys"})).unwrap(); + let (o1, i1) = (at(&s, outer), at(&s, inner)); + assert_ne!(o1, o0); + s.execute("comp.addMarker", json!({"comp": inner.0, "time": 1.0})).unwrap(); + assert_ne!(at(&s, inner), i1, "marker read by an expression"); + assert_ne!(at(&s, outer), o1, "marker read by an expression in a nested comp"); } +/// A protected time marker with a duration sets the comp's timing (Responsive Design), so it +/// changes the frames even though plain markers do not; and any expression, which can read +/// markers, makes every marker count. #[test] -fn markers_count_when_an_expression_can_read_them() { +fn timing_markers_and_expressions_still_change_the_frames() { use effectcraft_engine::Session; use serde_json::json; let mut s = Session::default(); let id = |v: serde_json::Value, k: &str| v[k].as_u64().unwrap(); let outer = ItemId(id(s.execute("comp.new", json!({"name": "Outer", "width": 8, "height": 8, "duration": 1})).unwrap(), "comp")); let solid = id(s.execute("layer.newSolid", json!({"comp": outer.0, "color": "#ff0000"})).unwrap(), "layer"); - s.execute("prop.setExpression", json!({"comp": outer.0, "layer": solid, "path": "transform/opacity", "expression": "1"})).unwrap(); let f = Frames::default(); let at = |s: &Session| f.content_of(&s.project, s.revision, outer); let o0 = at(&s); s.execute("layer.addMarker", json!({"comp": outer.0, "layers": [solid], "time": 0.5, "comment": "beat"})).unwrap(); - assert_ne!(at(&s), o0, "an expression can read the marker, so the frames change"); + assert_eq!(at(&s), o0, "a plain marker changes no pixels, so it keeps the frames"); + s.execute("comp.addMarker", json!({"comp": outer.0, "time": 0.25})).unwrap(); + s.execute("markers.set", json!({"comp": outer.0, "index": 0, "duration": 0.25, "protected": true})).unwrap(); + let o1 = at(&s); + assert_ne!(o1, o0, "a protected time marker with a duration sets the timing"); + s.execute("prop.setExpression", json!({"comp": outer.0, "layer": solid, "path": "transform/opacity", "expression": "1"})).unwrap(); + let o2 = at(&s); + s.execute("layer.addMarker", json!({"comp": outer.0, "layers": [solid], "time": 0.75})).unwrap(); + assert_ne!(at(&s), o2, "an expression can read the marker, so the frames change"); } diff --git a/crates/ui-egui/tests/it/ui_audio_scrub.rs b/crates/ui-egui/tests/it/ui_audio_scrub.rs index 8596d2506..a1f0a887b 100644 --- a/crates/ui-egui/tests/it/ui_audio_scrub.rs +++ b/crates/ui-egui/tests/it/ui_audio_scrub.rs @@ -651,3 +651,31 @@ fn startup_gives_up_on_a_first_frame_that_never_caches() { assert!(fake.0.lock().unwrap().0.is_none(), "no sound started and dropped"); assert!(h.state().playback.playing && h.state().playback.audio_held, "it goes on as a stalled preview"); } + +/// Adding markers while a preview sounds (Num*, one per beat) keeps the sound going (#1083): a +/// marker changes no pixels, so no cached frame is dropped and no frame has to be rendered again +/// (which would stop the sound until the frames ahead are cached). +#[test] +fn adding_markers_does_not_stop_the_sound() { + let fake = Fake::default(); + let (app, cid) = setup(true, &fake); + // Everything is cached before the preview starts, so the sound starts at once. + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).with_step_dt(1.0 / 31.0).build_eframe(|_| app); + h.run_steps(2); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60); + let now = h.ctx.input(|i| i.time); + h.state_mut().play(now); + while h.state().audio.is_none() && std::time::Instant::now() < deadline { + h.step(); + std::thread::sleep(std::time::Duration::from_millis(1)); + } + assert!(h.state().audio.is_some(), "the sound started"); + for beat in 0..6 { + let t = 0.1 + 0.05 * beat as f64; + h.state_mut().session.execute("comp.addMarker", json!({"comp": cid.0, "time": t, "comment": "beat"})).unwrap(); + h.step(); + let app = h.state(); + assert!(app.playback.playing && app.audio.is_some() && !app.playback.audio_held, "marker {beat}: still playing with sound"); + } + assert_eq!(h.state().session.project.comp(cid).unwrap().markers.len(), 6); +}