Fix 3d: stroke vanished on commit (establish full buffer on first execute)

The brush commits via a GPU-canvas readback and relies on the action's first
execute() to SET raw_pixels from that readback — at that point raw_pixels is empty
(new keyframe) or the pre-stroke state, never something a dirty-rect diff can stamp
onto. My initial diff-only execute skipped (to avoid corruption), so the stroke
disappeared.

Fix: the action keeps the full post-edit buffer ONLY for the first execute (the
commit), assigning raw_pixels outright exactly like the old code; it's taken/dropped
immediately, so the action sitting in the undo stack still retains just the small
diff. Redo replays via the diff onto the now-resident base.

Also harden the diff itself for the blank-base case: `before_blank` lets apply_after
build from a transparent buffer (redo of a first stroke after undo-to-blank) and
apply_before restore to empty; the resident-base skip is kept only for non-blank
bases (faulted in before undo/redo). Tests cover commit/redo from empty.
This commit is contained in:
Skyler Lehmkuhl 2026-06-20 21:44:14 -04:00
parent c627533185
commit 31e23b0fc7
3 changed files with 74 additions and 17 deletions

View File

@ -31,6 +31,11 @@ pub struct RasterDiff {
before_region: Vec<u8>, before_region: Vec<u8>,
/// bbox-sized RGBA (`w*h*4`) of the region after the edit. /// bbox-sized RGBA (`w*h*4`) of the region after the edit.
after_region: Vec<u8>, after_region: Vec<u8>,
/// The pre-edit buffer was blank (empty/unallocated) — i.e. this was the first
/// edit on a fresh keyframe. Lets `apply_after` build from a transparent base
/// (the commit/redo path often starts with empty `raw_pixels`) and `apply_before`
/// restore to blank, instead of requiring a resident base.
before_blank: bool,
} }
impl RasterDiff { impl RasterDiff {
@ -39,6 +44,7 @@ impl RasterDiff {
/// keyframe's first stroke), treated as fully transparent. /// keyframe's first stroke), treated as fully transparent.
pub fn compute(before: &[u8], after: &[u8], width: u32, height: u32) -> Self { pub fn compute(before: &[u8], after: &[u8], width: u32, height: u32) -> Self {
let n = width as usize * height as usize * 4; let n = width as usize * height as usize * 4;
let before_blank = before.len() != n;
// Normalize both sides to full length; empty/short ⇒ transparent. // Normalize both sides to full length; empty/short ⇒ transparent.
let before_full = normalize(before, n); let before_full = normalize(before, n);
let after_full = normalize(after, n); let after_full = normalize(after, n);
@ -63,7 +69,7 @@ impl RasterDiff {
if !any { if !any {
return Self { full_width: width, full_height: height, bbox: None, return Self { full_width: width, full_height: height, bbox: None,
before_region: Vec::new(), after_region: Vec::new() }; before_region: Vec::new(), after_region: Vec::new(), before_blank };
} }
let bw = max_x - min_x + 1; let bw = max_x - min_x + 1;
@ -83,6 +89,7 @@ impl RasterDiff {
bbox: Some((min_x as u32, min_y as u32, bw as u32, bh as u32)), bbox: Some((min_x as u32, min_y as u32, bw as u32, bh as u32)),
before_region: crop(&before_full), before_region: crop(&before_full),
after_region: crop(&after_full), after_region: crop(&after_full),
before_blank,
} }
} }
@ -91,25 +98,45 @@ impl RasterDiff {
self.before_region.len() + self.after_region.len() self.before_region.len() + self.after_region.len()
} }
/// Restore the pre-edit pixels into `raw` (undo). /// Restore the pre-edit pixels into `raw` (undo / first-execute rollback).
pub fn apply_before(&self, raw: &mut Vec<u8>) { pub fn apply_before(&self, raw: &mut Vec<u8>) {
self.apply(&self.before_region, raw); if self.bbox.is_none() {
return; // no change
}
if self.before_blank {
// The frame was blank before this edit (it was the first stroke); undoing
// it returns to blank regardless of the current buffer.
raw.clear();
return;
}
self.stamp_resident(&self.before_region, raw);
} }
/// Re-apply the post-edit pixels into `raw` (redo). /// Apply the post-edit pixels into `raw` (commit / redo).
pub fn apply_after(&self, raw: &mut Vec<u8>) { pub fn apply_after(&self, raw: &mut Vec<u8>) {
self.apply(&self.after_region, raw); if self.bbox.is_none() {
return; // no change
}
if self.before_blank {
// Base was blank: build a full transparent buffer then stamp the bbox. The
// commit/redo path frequently starts from empty `raw_pixels` here.
let n = self.full_width as usize * self.full_height as usize * 4;
raw.clear();
raw.resize(n, 0);
}
self.stamp_resident(&self.after_region, raw);
} }
fn apply(&self, region: &[u8], raw: &mut Vec<u8>) { /// Stamp a bbox-sized region into `raw`, which must already be full-size. If it
/// isn't (a non-blank base that the editor failed to fault in), skip rather than
/// resize-and-corrupt — the frame will re-page to its container state.
fn stamp_resident(&self, region: &[u8], raw: &mut [u8]) {
let n = self.full_width as usize * self.full_height as usize * 4; let n = self.full_width as usize * self.full_height as usize * 4;
let (x, y, bw, bh) = match self.bbox { let (x, y, bw, bh) = match self.bbox {
Some(b) => b, Some(b) => b,
None => return, // no change None => return,
}; };
if raw.len() != n { if raw.len() != n {
// Base not resident: the editor should have faulted it in. Skip rather
// than resize-and-corrupt — the frame will re-page to its container state.
eprintln!( eprintln!(
"⚠️ [RASTER_DIFF] base not resident ({} != {}); skipping undo/redo apply", "⚠️ [RASTER_DIFF] base not resident ({} != {}); skipping undo/redo apply",
raw.len(), n raw.len(), n
@ -157,17 +184,27 @@ mod tests {
#[test] #[test]
fn blank_before_first_stroke() { fn blank_before_first_stroke() {
let (w, h) = (4, 4); let (w, h) = (4, 4);
let n = (w * h * 4) as usize;
let before: Vec<u8> = Vec::new(); // blank keyframe let before: Vec<u8> = Vec::new(); // blank keyframe
let mut after = vec![0u8; (w * h * 4) as usize]; let mut after = vec![0u8; n];
let i = ((1 * w + 1) * 4) as usize; let i = ((1 * w + 1) * 4) as usize;
after[i..i + 4].copy_from_slice(&[255, 0, 0, 255]); after[i..i + 4].copy_from_slice(&[255, 0, 0, 255]);
let diff = RasterDiff::compute(&before, &after, w, h); let diff = RasterDiff::compute(&before, &after, w, h);
assert_eq!(diff.bbox, Some((1, 1, 1, 1))); assert_eq!(diff.bbox, Some((1, 1, 1, 1)));
// Undo onto the resident post-stroke buffer → fully transparent (blank-equiv). // First execute / redo from EMPTY raw_pixels (the real commit path): builds
let mut buf = after.clone(); // the full buffer from transparent + the stroke.
let mut buf: Vec<u8> = Vec::new();
diff.apply_after(&mut buf);
assert_eq!(buf, after, "commit/redo must build the frame from a blank base");
// Undo the first stroke → back to blank (empty).
diff.apply_before(&mut buf); diff.apply_before(&mut buf);
assert_eq!(buf, vec![0u8; (w * h * 4) as usize]); assert!(buf.is_empty(), "undoing the first stroke restores the blank keyframe");
// Redo again from the now-empty buffer.
diff.apply_after(&mut buf);
assert_eq!(buf, after);
} }
#[test] #[test]

View File

@ -13,6 +13,9 @@ pub struct RasterFillAction {
height: u32, height: u32,
name: String, name: String,
diff: RasterDiff, diff: RasterDiff,
/// Full post-fill buffer, kept only for the first `execute` (commit); see
/// `RasterStrokeAction::full_after`.
full_after: Option<Vec<u8>>,
} }
impl RasterFillAction { impl RasterFillAction {
@ -25,7 +28,8 @@ impl RasterFillAction {
height: u32, height: u32,
) -> Self { ) -> Self {
let diff = RasterDiff::compute(&buffer_before, &buffer_after, width, height); let diff = RasterDiff::compute(&buffer_before, &buffer_after, width, height);
Self { layer_id, time, width, height, name: "Flood fill".to_string(), diff } Self { layer_id, time, width, height, name: "Flood fill".to_string(),
diff, full_after: Some(buffer_after) }
} }
pub fn with_description(mut self, name: &str) -> Self { pub fn with_description(mut self, name: &str) -> Self {
@ -43,7 +47,11 @@ impl Action for RasterFillAction {
_ => return Err("Not a raster layer".to_string()), _ => return Err("Not a raster layer".to_string()),
}; };
let kf = raster.ensure_keyframe_at(self.time, self.width, self.height); let kf = raster.ensure_keyframe_at(self.time, self.width, self.height);
self.diff.apply_after(&mut kf.raw_pixels); if let Some(full) = self.full_after.take() {
kf.raw_pixels = full;
} else {
self.diff.apply_after(&mut kf.raw_pixels);
}
kf.texture_dirty = true; kf.texture_dirty = true;
kf.dirty = true; kf.dirty = true;
Ok(()) Ok(())

View File

@ -25,6 +25,12 @@ pub struct RasterStrokeAction {
width: u32, width: u32,
height: u32, height: u32,
diff: RasterDiff, diff: RasterDiff,
/// The full post-stroke buffer, kept ONLY for the first `execute` (the commit),
/// which establishes `raw_pixels` exactly like the old code did — robust no matter
/// what state the working buffer is in (empty new keyframe, GPU-canvas readback,
/// etc.). Taken (dropped) on first execute, so the action sitting in the undo stack
/// retains only the small `diff`; redo then replays via the diff.
full_after: Option<Vec<u8>>,
} }
impl RasterStrokeAction { impl RasterStrokeAction {
@ -43,14 +49,20 @@ impl RasterStrokeAction {
height: u32, height: u32,
) -> Self { ) -> Self {
let diff = RasterDiff::compute(&buffer_before, &buffer_after, width, height); let diff = RasterDiff::compute(&buffer_before, &buffer_after, width, height);
Self { layer_id, time, width, height, diff } Self { layer_id, time, width, height, diff, full_after: Some(buffer_after) }
} }
} }
impl Action for RasterStrokeAction { impl Action for RasterStrokeAction {
fn execute(&mut self, document: &mut Document) -> Result<(), String> { fn execute(&mut self, document: &mut Document) -> Result<(), String> {
let kf = get_keyframe_mut(document, &self.layer_id, self.time, self.width, self.height)?; let kf = get_keyframe_mut(document, &self.layer_id, self.time, self.width, self.height)?;
self.diff.apply_after(&mut kf.raw_pixels); if let Some(full) = self.full_after.take() {
// First execute (commit): assign the full buffer outright.
kf.raw_pixels = full;
} else {
// Redo: replay via the diff onto the (resident) base.
self.diff.apply_after(&mut kf.raw_pixels);
}
kf.texture_dirty = true; kf.texture_dirty = true;
kf.dirty = true; kf.dirty = true;
Ok(()) Ok(())