diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/set_keyframe.rs b/lightningbeam-ui/lightningbeam-core/src/actions/set_keyframe.rs index b2bb8dc..8f10b6f 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/set_keyframe.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/set_keyframe.rs @@ -8,6 +8,7 @@ use crate::action::Action; use crate::animation::{AnimationCurve, AnimationTarget, Keyframe, TransformProperty}; use crate::document::Document; use crate::layer::{AnyLayer, ShapeKeyframe}; +use crate::object::Transform; use uuid::Uuid; /// Undo info for a clip animation curve @@ -54,11 +55,17 @@ const TRANSFORM_PROPERTIES: &[TransformProperty] = &[ TransformProperty::Opacity, ]; -fn transform_default(prop: &TransformProperty) -> f64 { +/// The clip instance's own value for a property (its base transform / opacity). +fn transform_prop_value(t: &Transform, opacity: f64, prop: &TransformProperty) -> f64 { match prop { - TransformProperty::ScaleX | TransformProperty::ScaleY => 1.0, - TransformProperty::Opacity => 1.0, - _ => 0.0, + TransformProperty::X => t.x, + TransformProperty::Y => t.y, + TransformProperty::Rotation => t.rotation, + TransformProperty::ScaleX => t.scale_x, + TransformProperty::ScaleY => t.scale_y, + TransformProperty::SkewX => t.skew_x, + TransformProperty::SkewY => t.skew_y, + TransformProperty::Opacity => opacity, } } @@ -67,6 +74,21 @@ impl Action for SetKeyframeAction { self.clip_undo_entries.clear(); self.shape_keyframe_created = false; + // Phase 1 (immutable): for each clip instance, gather its base transform and the + // start time of its visibility region, so a brand-new curve can be anchored there. + let mut clip_info: std::collections::HashMap = + std::collections::HashMap::new(); // id -> (base transform, opacity, start time) + if let Some(AnyLayer::Vector(vl)) = document.get_layer(&self.layer_id) { + for clip_id in &self.clip_instance_ids { + if let Some(ci) = vl.clip_instances.iter().find(|c| c.id == *clip_id) { + let start = vl + .group_visibility_start(clip_id, self.time) + .unwrap_or(ci.timeline_start); + clip_info.insert(*clip_id, (ci.transform.clone(), ci.opacity, start)); + } + } + } + let layer = document .get_layer_mut(&self.layer_id) .ok_or_else(|| format!("Layer {} not found", self.layer_id))?; @@ -82,23 +104,37 @@ impl Action for SetKeyframeAction { // Add clip animation keyframes for clip_id in &self.clip_instance_ids { + let (base_transform, base_opacity, start) = clip_info + .get(clip_id) + .cloned() + .unwrap_or((Transform::new(), 1.0, 0.0)); for prop in TRANSFORM_PROPERTIES { let target = AnimationTarget::Object { id: *clip_id, property: *prop, }; - let default = transform_default(prop); - let value = vl.layer.animation_data.eval(&target, self.time, default); + // Fall back to the clip's OWN value (not a generic default) so a brand-new + // keyframe captures the actual on-stage position, not (0,0)/identity. + let base = transform_prop_value(&base_transform, base_opacity, prop); + let value = vl.layer.animation_data.eval(&target, self.time, base); let curve_created = vl.layer.animation_data.get_curve(&target).is_none(); if curve_created { vl.layer .animation_data - .set_curve(AnimationCurve::new(target.clone(), default)); + .set_curve(AnimationCurve::new(target.clone(), base)); } let curve = vl.layer.animation_data.get_curve_mut(&target).unwrap(); let old_keyframe = curve.get_keyframe_at(self.time, 0.001).cloned(); + + // When this is the first keyframe of the curve and the clip already existed + // before `time`, anchor a keyframe at its start with the original value. + // Otherwise a single keyframe would Hold-extrapolate backward and move the + // clip on every earlier frame too (the motion-tween first-keyframe bug). + if curve_created && start < self.time - 0.001 { + curve.set_keyframe(Keyframe::linear(start, base)); + } curve.set_keyframe(Keyframe::linear(self.time, value)); self.clip_undo_entries.push(ClipUndoEntry { @@ -145,3 +181,86 @@ impl Action for SetKeyframeAction { "New keyframe".to_string() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::actions::TransformClipInstancesAction; + use crate::clip::ClipInstance; + use crate::layer::VectorLayer; + use std::collections::HashMap; + + fn x_curve_eval(document: &Document, layer_id: Uuid, instance_id: Uuid, time: f64) -> f64 { + let target = AnimationTarget::Object { id: instance_id, property: TransformProperty::X }; + match document.get_layer(&layer_id) { + Some(AnyLayer::Vector(vl)) => vl.layer.animation_data.eval(&target, time, f64::NAN), + _ => panic!("no layer"), + } + } + + #[test] + fn first_keyframe_then_move_does_not_disturb_earlier_frames() { + // Group created at frame 0 (clip at x=50), keyframe + move at frame 10 → x=200. + // Frame 0 must keep x=50 (the motion-tween first-keyframe bug: it used to become 200). + let mut document = Document::new("Test"); + let mut layer = VectorLayer::new("Layer"); + let clip_id = Uuid::new_v4(); + let instance_id = Uuid::new_v4(); + let mut instance = ClipInstance::with_id(instance_id, clip_id); + instance.transform = Transform::with_position(50.0, 50.0); + layer.clip_instances.push(instance); + // The group's visibility starts at a keyframe at time 0 containing the instance. + layer.ensure_keyframe_at(0.0).clip_instance_ids.push(instance_id); + let layer_id = document.root_mut().add_child(AnyLayer::Vector(layer)); + + // Create a keyframe at frame 10. + SetKeyframeAction::new(layer_id, 10.0, vec![instance_id]) + .execute(&mut document) + .unwrap(); + + // The new curve must be anchored at the start (two keyframes, both at x=50 so far). + assert!((x_curve_eval(&document, layer_id, instance_id, 0.0) - 50.0).abs() < 1e-6); + assert!((x_curve_eval(&document, layer_id, instance_id, 10.0) - 50.0).abs() < 1e-6); + + // Move the clip at frame 10 to x=200. + let mut transforms = HashMap::new(); + transforms.insert( + instance_id, + (Transform::with_position(50.0, 50.0), Transform::with_position(200.0, 200.0)), + ); + TransformClipInstancesAction::new(layer_id, 10.0, transforms) + .execute(&mut document) + .unwrap(); + + // Frame 0 unchanged; frame 10 moved; midpoint tweens. + assert!((x_curve_eval(&document, layer_id, instance_id, 0.0) - 50.0).abs() < 1e-6, "frame 0 must stay 50"); + assert!((x_curve_eval(&document, layer_id, instance_id, 10.0) - 200.0).abs() < 1e-6, "frame 10 must be 200"); + assert!((x_curve_eval(&document, layer_id, instance_id, 5.0) - 125.0).abs() < 1e-6, "midpoint tweens"); + } + + #[test] + fn first_keyframe_at_clip_start_is_not_double_anchored() { + // When the keyframe is created at the clip's own start, there's nothing earlier to + // anchor — a single keyframe is correct. + let mut document = Document::new("Test"); + let mut layer = VectorLayer::new("Layer"); + let clip_id = Uuid::new_v4(); + let instance_id = Uuid::new_v4(); + let mut instance = ClipInstance::with_id(instance_id, clip_id); + instance.transform = Transform::with_position(10.0, 0.0); + layer.clip_instances.push(instance); + layer.ensure_keyframe_at(0.0).clip_instance_ids.push(instance_id); + let layer_id = document.root_mut().add_child(AnyLayer::Vector(layer)); + + SetKeyframeAction::new(layer_id, 0.0, vec![instance_id]) + .execute(&mut document) + .unwrap(); + + let target = AnimationTarget::Object { id: instance_id, property: TransformProperty::X }; + if let Some(AnyLayer::Vector(vl)) = document.get_layer(&layer_id) { + let curve = vl.layer.animation_data.get_curve(&target).unwrap(); + assert_eq!(curve.keyframes.len(), 1, "keyframe at clip start needs no anchor"); + assert!((curve.eval(0.0) - 10.0).abs() < 1e-6); + } + } +} diff --git a/lightningbeam-ui/lightningbeam-core/src/layer.rs b/lightningbeam-ui/lightningbeam-core/src/layer.rs index 821bbe6..6b15ffa 100644 --- a/lightningbeam-ui/lightningbeam-core/src/layer.rs +++ b/lightningbeam-ui/lightningbeam-core/src/layer.rs @@ -433,6 +433,25 @@ impl VectorLayer { time + frame_duration } + /// Start time of the group clip instance's visibility region that contains `time`: + /// the time of the earliest keyframe reachable by walking back through consecutive + /// keyframes that all contain the clip instance. Returns `None` if the clip instance + /// isn't a keyframe-gated group visible at `time` (e.g. a movie clip). + pub fn group_visibility_start(&self, clip_instance_id: &Uuid, time: f64) -> Option { + let after = self.keyframes.partition_point(|kf| kf.time <= time); + if after == 0 { + return None; + } + let mut idx = after - 1; // keyframe at-or-before `time` + if !self.keyframes[idx].clip_instance_ids.contains(clip_instance_id) { + return None; + } + while idx > 0 && self.keyframes[idx - 1].clip_instance_ids.contains(clip_instance_id) { + idx -= 1; + } + Some(self.keyframes[idx].time) + } + // Shape-based methods removed — use DCEL methods instead. // - shapes_at_time_mut → graph_at_time_mut // - get_shape_in_keyframe → use DCEL vertex/edge/face accessors