diff --git a/AGENTS.md b/AGENTS.md index 60c5ddd..88b0a3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,7 +62,7 @@ Commands registered on the **`docgen`** CLI include: ## Implications for changes here - **Manim / `scenes.py` (marker blocks):** Fix generators under `src/docgen/**` (`manim_scene_support.py`, `scene_spec.py`, `scene_spec_generate.py`, `validate`, `yaml_generate`, tests). **Do not** patch generated classes inside a consumer's **`animations/scenes.py`** between **`BEGIN/END GENERATED SCENE`** markers; re-run **`scene-spec-generate`** / **`scene-compile --retime`** and **`manim`** instead. Preferred consumer order: narration → TTS → timestamps → scene-spec/compile → Manim → compose. -- **Beat sync (fail-closed):** when `timing.json` has words, every story box label must match a spoken phrase (`wait_word`); unmatched labels and leftover LLM indices are rejected. Opt out with ``pace: none``. Fuzzy containment matching is not used. +- **Beat sync (fail-closed):** when `timing.json` has words, every story box label must match a spoken phrase (`wait_word`); unmatched labels and leftover LLM indices are rejected. Opt out with ``pace: none``. Fuzzy containment matching is not used. **`scene-compile` clamps FadeIn / page-fade `run_time` against the next word start** so `_TimedScene._clock` cannot race past waits (issue #66 — do not emit cascading first-board dumps). Page transitions FadeOut revealed boxes, not the parent `VGroup`. - **Subject-beat coverage:** implemented in `scene_spec.layout_density_violations` / `cluster_subject_beats`; enforced by **`scene-spec-generate`** and **`validate`** (`validation.subject_beat_coverage.enabled`, default true). Not a blind label count. - Prefer **stable CLI / library contracts** and **documented exit codes** so CI can depend on them. - **`narration_from_source`:** hints in config + **`docgen narration-generate`** — owner-supplied context paths, not opaque bulk edits to outputs. diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index fd54227..6dd56d3 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -243,9 +243,23 @@ class _TimedScene(Scene): def setup(self): self._clock = 0.0 - def timed_play(self, *animations, run_time=1.0, **kwargs): - self.play(*animations, run_time=run_time, **kwargs) - self._clock += run_time + def timed_play(self, *animations, run_time=1.0, not_past=None, **kwargs): + """Play animations and advance ``_clock``. + + Optional ``not_past`` (absolute seconds) clamps ``run_time`` so the clock + cannot race past the next paced reveal — defense in depth when compiled + run_times were not clamped (issue #66). + """ + rt = float(run_time) + if not_past is not None: + try: + limit = float(not_past) + except (TypeError, ValueError): + limit = None + if limit is not None and self._clock + rt > limit: + rt = max(0.25, limit - self._clock) + self.play(*animations, run_time=rt, **kwargs) + self._clock += rt def wait_until(self, target: float): gap = target - self._clock diff --git a/src/docgen/scene_spec.py b/src/docgen/scene_spec.py index e1e1e7c..a18f763 100644 --- a/src/docgen/scene_spec.py +++ b/src/docgen/scene_spec.py @@ -33,6 +33,7 @@ from __future__ import annotations import re +from dataclasses import dataclass from pathlib import Path from typing import Any @@ -40,6 +41,13 @@ _SENTENCE_SPLIT_RE = re.compile(r"(?<=[.!?])\s+") +# Compiled scenes Write the title then pace boxes with wait_until_word against +# ``_TimedScene._clock``. Keep the title short so early Whisper starts are not +# already in the past before the first box reveal. +TITLE_WRITE_RUN_TIME = 1.0 +# Floor when clamping FadeIn so the clock cannot overshoot the next wait_word. +MIN_REVEAL_RUN_TIME = 0.25 + ALLOWED_COLORS = frozenset( { "C_BG", @@ -656,6 +664,255 @@ def last_paced_reveal_time( return max(t for _, t in anchors) +@dataclass(frozen=True) +class RevealEvent: + """One box reveal on the compiled ``_TimedScene`` clock timeline.""" + + label: str + page: int + row: int + box: int + wait_word: int | None + word_start: float | None + effective_at: float + wait_skipped: bool + run_time: float + page_fade_out: float + + +def _box_wait_word(row: dict[str, Any], box: dict[str, Any], box_index: int) -> int | None: + if _pace_none(row) or _pace_none(box): + return None + ww = box.get("wait_word") + if ww is None and box_index == 0: + ww = row.get("wait_word") + if ww is None: + return None + try: + return int(ww) + except (TypeError, ValueError): + return None + + +def _word_start_at(words: list[dict[str, Any]], index: int | None) -> float | None: + if index is None or not words or index < 0 or index >= len(words): + return None + w = words[index] + if not isinstance(w, dict): + return None + try: + return float(w.get("start", 0.0)) + except (TypeError, ValueError): + return None + + +def iter_reveal_slots( + spec: dict[str, Any], + words: list[dict[str, Any]] | None = None, +) -> list[dict[str, Any]]: + """Return paced/unpaced story boxes in compiled reveal order. + + Each slot is a dict with page/row/box indices, label, wait_word, word_start, + row run_time, and page transition metadata for the first box of each page. + When ``words`` is provided, ``wait_word`` is re-derived via fail-closed sync. + """ + working = ( + sync_row_labels_to_whisper_words(spec, words, overwrite=True) + if words + else spec + ) + pages = _normalized_pages(working) + layout = working.get("layout") or {} + default_tr = str(layout.get("page_transition", "fade")) + default_tr_rt = float(layout.get("page_transition_run_time", 0.45)) + slots: list[dict[str, Any]] = [] + for p, page in enumerate(pages): + if not isinstance(page, dict): + continue + rows = page.get("rows") + if not isinstance(rows, list): + continue + trans = page.get("transition", default_tr) + for r, row in enumerate(rows): + if not isinstance(row, dict): + continue + boxes = row.get("boxes") + if not isinstance(boxes, list): + continue + try: + row_rt = float(row.get("run_time", 1.0)) + except (TypeError, ValueError): + row_rt = 1.0 + for b, box in enumerate(boxes): + if not isinstance(box, dict): + continue + if box.get("image") is not None and not str(box.get("label", "")).strip(): + # Unlabeled image: still revealed, but no spoken wait. + label = "" + else: + label = str(box.get("label", "")).strip() + ww = _box_wait_word(row, box, b) + slots.append( + { + "page": p, + "row": r, + "box": b, + "label": label, + "wait_word": ww, + "word_start": _word_start_at(words or [], ww), + "run_time": row_rt, + "page_transition": str(trans or default_tr), + "page_transition_run_time": default_tr_rt, + } + ) + return slots + + +def simulate_reveal_timeline( + spec: dict[str, Any], + words: list[dict[str, Any]], + *, + title_run_time: float = TITLE_WRITE_RUN_TIME, + clamp_run_times: bool = True, +) -> list[RevealEvent]: + """Simulate ``_TimedScene`` clock for compiled wait_word + FadeIn sequences. + + When ``clamp_run_times`` is True (compile default), FadeIn / page FadeOut + durations shrink so ``_clock`` cannot pass the next paced word start — the + bug that dumped the first board then froze while narration continued. + """ + slots = iter_reveal_slots(spec, words) + if not slots: + return [] + + clock = float(title_run_time) + events: list[RevealEvent] = [] + + for i, slot in enumerate(slots): + word_start = slot["word_start"] + ww = slot["wait_word"] + wait_skipped = False + if ww is not None and word_start is not None: + target = float(word_start) + if target > clock + 0.05: + clock = target + elif clock > target + 0.05: + # Truly late: prior FadeIn/title already passed this spoken start. + wait_skipped = True + else: + # On-time within tolerance (clamp landed on the beat). + clock = max(clock, target) + + page_fade_out = 0.0 + if ( + int(slot["page"]) > 0 + and int(slot["row"]) == 0 + and int(slot["box"]) == 0 + and str(slot["page_transition"]) == "fade" + ): + page_fade_out = float(slot["page_transition_run_time"]) + + next_target: float | None = None + for nxt in slots[i + 1 :]: + if nxt["wait_word"] is not None and nxt["word_start"] is not None: + next_target = float(nxt["word_start"]) + break + + rt = float(slot["run_time"]) + if clamp_run_times and next_target is not None: + budget = next_target - clock + if page_fade_out + rt > budget and budget > 0: + # Prefer keeping a short FadeIn; shrink page fade first. + if page_fade_out > 0 and page_fade_out > max(0.0, budget - MIN_REVEAL_RUN_TIME): + page_fade_out = max(0.05, budget - MIN_REVEAL_RUN_TIME) + remain = next_target - (clock + page_fade_out) + if remain < rt: + rt = max(MIN_REVEAL_RUN_TIME, remain) + elif budget <= 0: + page_fade_out = 0.05 if page_fade_out > 0 else 0.0 + rt = MIN_REVEAL_RUN_TIME + + if page_fade_out > 0: + clock += page_fade_out + + events.append( + RevealEvent( + label=str(slot["label"]), + page=int(slot["page"]), + row=int(slot["row"]), + box=int(slot["box"]), + wait_word=ww, + word_start=word_start, + effective_at=float(clock), + wait_skipped=wait_skipped, + run_time=float(rt), + page_fade_out=float(page_fade_out), + ) + ) + clock += rt + + return events + + +def reveal_cadence_violations( + events: list[RevealEvent], + *, + audio_end: float, + max_skip_ratio: float = 0.25, + max_consecutive_skips: int = 2, + max_early_sec: float = 40.0, + max_early_ratio: float = 0.45, +) -> list[str]: + """Return issues when the simulated clock dumps boxes then idles. + + Checks (issue #66): + + * too many ``wait_until_word`` no-ops (``_clock`` already past word start) + * long consecutive skip streaks (rapid cascade) + * last **effective** reveal finishes far before ``audio_end`` (metadata-only + ``story_end`` misses this when Whisper starts look late but Manim raced) + """ + if not events or audio_end <= 0: + return [] + + issues: list[str] = [] + paced = [e for e in events if e.wait_word is not None] + if paced: + skipped = sum(1 for e in paced if e.wait_skipped) + skip_ratio = skipped / len(paced) + if skip_ratio > max_skip_ratio and skipped >= 2: + issues.append( + f"wait_until_word no-op ratio {skip_ratio:.0%} ({skipped}/{len(paced)}) " + f"exceeds max_skip_ratio={max_skip_ratio:.0%} — FadeIn/title run_time " + "pushed _clock past spoken word starts (first board dumps, then freezes)" + ) + streak = 0 + best = 0 + for e in paced: + if e.wait_skipped: + streak += 1 + best = max(best, streak) + else: + streak = 0 + if best > max_consecutive_skips: + issues.append( + f"{best} consecutive skipped waits (max_consecutive_skips=" + f"{max_consecutive_skips}) — boxes cascade with only FadeIn gaps" + ) + + last_effective = max(e.effective_at for e in events) + early_idle = audio_end - last_effective + early_ratio = early_idle / audio_end if audio_end > 0 else 0.0 + if early_idle > max_early_sec and early_ratio > max_early_ratio: + issues.append( + f"effective last reveal at {last_effective:.2f}s leaves " + f"early_idle={early_idle:.2f}s ({early_ratio:.0%} of audio_end=" + f"{audio_end:.2f}s) — visual story finished on the Manim clock long " + "before narration ends" + ) + return issues + + def upgrade_wait_segments_to_wait_words( spec: dict[str, Any], words: list[dict[str, Any]], @@ -1388,11 +1645,20 @@ def _any_wait_segment_in_pages(pages: list[dict[str, Any]]) -> bool: return False -def compile_scene_class(spec: dict[str, Any]) -> str: +def compile_scene_class( + spec: dict[str, Any], + *, + words: list[dict[str, Any]] | None = None, +) -> str: """Return a full ``class Name(_TimedScene): ...`` definition (no imports). ``spec`` must include ``timing_key`` (narration audio stem for ``timing.json``), either in the mapping or merged by the caller from ``Config.resolve_segment_name``. + + When ``words`` is provided (normal ``scene-compile`` / retime path), FadeIn and + page-transition durations are **clamped** so ``_TimedScene._clock`` cannot race + past the next ``wait_word`` start — otherwise the first board dumps and freezes + while narration continues (issue #66). """ validate_scene_spec(spec, path_label="spec") @@ -1425,6 +1691,13 @@ def compile_scene_class(spec: dict[str, Any]) -> str: "words, or edit the YAML to use wait_word." ) + # Per-box clamped run_time / page fade when Whisper words are available. + reveal_by_key: dict[tuple[int, int, int], RevealEvent] = {} + if words: + for ev in simulate_reveal_timeline(spec, words, clamp_run_times=True): + reveal_by_key[(ev.page, ev.row, ev.box)] = ev + + title_rt = TITLE_WRITE_RUN_TIME lines: list[str] = [ f"class {class_name}(_TimedScene):", " def construct(self):", @@ -1440,7 +1713,7 @@ def compile_scene_class(spec: dict[str, Any]) -> str: f" _title_sub = Text({title_subtitle!r}, font_size={sub_fs}, color={title_color})", " _title_sub.set_opacity(0.85)", " title = VGroup(_title_main, _title_sub).arrange(DOWN, buff=0.12).to_edge(UP)", - " self.timed_play(Write(title), run_time=2.0)", + f" self.timed_play(Write(title), run_time={title_rt})", "", ] ) @@ -1448,7 +1721,7 @@ def compile_scene_class(spec: dict[str, Any]) -> str: lines.extend( [ f" title = Text({title_text!r}, font_size={title_fs}, color={title_color}).to_edge(UP)", - " self.timed_play(Write(title), run_time=2.0)", + f" self.timed_play(Write(title), run_time={title_rt})", "", ] ) @@ -1553,16 +1826,37 @@ def compile_scene_class(spec: dict[str, Any]) -> str: lines.append("") + # Box vars per page — page transitions FadeOut these individually. Fading the + # parent VGroup can re-add unrevealed siblings at full opacity (flash dump). + page_box_vars: dict[int, list[str]] = {} + for p, page in enumerate(pages): + for r, row in enumerate(page["rows"]): + boxes_raw = row.get("boxes") or [] + if not isinstance(boxes_raw, list): + continue + for b_idx, box in enumerate(boxes_raw): + if isinstance(box, dict): + page_box_vars.setdefault(p, []).append(f"_bx_{p}_{r}_{b_idx}") + for p, page in enumerate(pages): for r, row in enumerate(page["rows"]): boxes_raw = row["boxes"] if not isinstance(boxes_raw, list): continue - run_time = float(row["run_time"]) + row_run_time = float(row["run_time"]) row_ww = row.get("wait_word") for b_idx, box in enumerate(boxes_raw): if not isinstance(box, dict): continue + ev = reveal_by_key.get((p, r, b_idx)) + run_time = ( + round(float(ev.run_time), 3) if ev is not None else row_run_time + ) + page_fade_rt = ( + round(float(ev.page_fade_out), 3) + if ev is not None + else page_tr_run + ) ww = box.get("wait_word") if ww is None and b_idx == 0 and row_ww is not None: ww = row_ww @@ -1572,14 +1866,18 @@ def compile_scene_class(spec: dict[str, Any]) -> str: ) if p > 0 and r == 0 and b_idx == 0: trans = page.get("transition") - prev_stack = f"_p{p - 1}_stack" + prev_boxes = page_box_vars.get(p - 1) or [] prev_edges = page_edge_vars.get(p - 1) or [] - fade_targets = [prev_stack] + prev_edges - fade_args = ", ".join(f"FadeOut({t})" for t in fade_targets) + fade_targets = prev_boxes + prev_edges if trans == "fade": - lines.append( - f" self.timed_play({fade_args}, run_time={page_tr_run})" - ) + if fade_targets: + fade_args = ", ".join(f"FadeOut({t})" for t in fade_targets) + lines.append( + f" self.timed_play({fade_args}, " + f"run_time={page_fade_rt})" + ) + else: + lines.append(f" self.timed_wait({page_fade_rt})") elif trans == "none": for t in fade_targets: lines.append(f" self.remove({t})") @@ -1588,11 +1886,11 @@ def compile_scene_class(spec: dict[str, Any]) -> str: edge_anims = edges_with_target.get((p, bx), []) if edge_anims: parts = [f"FadeIn({bx})"] - for ev, kind in edge_anims: + for evar, kind in edge_anims: if kind == "grow": - parts.append(f"GrowArrow({ev})") + parts.append(f"GrowArrow({evar})") else: - parts.append(f"FadeIn({ev})") + parts.append(f"FadeIn({evar})") anims = ", ".join(parts) lines.append( f" self.timed_play({anims}, run_time={run_time})" diff --git a/src/docgen/scene_spec_generate.py b/src/docgen/scene_spec_generate.py index 6ec0394..e58d1e8 100644 --- a/src/docgen/scene_spec_generate.py +++ b/src/docgen/scene_spec_generate.py @@ -365,7 +365,9 @@ def linted_class_block_from_spec( ) try: - class_block = compile_scene_class(merged) + # Pass Whisper words so compile clamps FadeIn/page-fade run_times against + # the next wait_word (issue #66 — do not emit clock-racing garbage). + class_block = compile_scene_class(merged, words=words or None) except SceneSpecError as exc: raise SceneGenerationError(str(exc)) from exc issues = lint_generated_block( diff --git a/tests/test_scene_spec.py b/tests/test_scene_spec.py index 899fe0d..6f8c578 100644 --- a/tests/test_scene_spec.py +++ b/tests/test_scene_spec.py @@ -7,6 +7,8 @@ import pytest from docgen.scene_spec import ( + MIN_REVEAL_RUN_TIME, + TITLE_WRITE_RUN_TIME, SceneSpecError, auto_fit_row_widths, auto_paginate, @@ -23,7 +25,9 @@ narration_sentence_count, narration_sentences, pacing_violations, + reveal_cadence_violations, segment_index_for_whisper_time, + simulate_reveal_timeline, sync_row_labels_to_whisper_words, validate_scene_spec, ) @@ -99,8 +103,11 @@ def test_multi_page_emits_transition_and_second_stack() -> None: out = compile_scene_class(spec) assert "timing_words = _load_timing_words('01-x')" in out assert "_p1_stack" in out - assert "self.timed_play(FadeOut(_p0_stack), run_time=0.5)" in out + # Fade out revealed boxes (not the parent VGroup — that re-adds siblings). + assert "FadeOut(_bx_0_0_0)" in out + assert "FadeOut(_p0_stack)" not in out assert "FadeIn(_bx_1_0_0)" in out + assert f"Write(title), run_time={TITLE_WRITE_RUN_TIME}" in out def test_validate_rejects_rows_and_pages_together() -> None: @@ -1155,3 +1162,92 @@ def test_validate_edges_requires_known_labels() -> None: "edges": [{"from": "A", "to": "Missing"}], } ) + + +def _cascade_spec() -> dict: + """Three paced boxes with long FadeIn run_times vs tight Whisper gaps.""" + return { + "segment_id": "01", + "class_name": "CascadeScene", + "timing_key": "01-cascade", + "title": {"text": "T", "font_size": 36, "color": "C_WHITE"}, + "rows": [ + { + "run_time": 1.5, + "boxes": [ + { + "label": "Alpha", + "color": "C_GREEN", + "width": 3.0, + "height": 0.8, + "font_size": 18, + "wait_word": 0, + }, + { + "label": "Beta", + "color": "C_BLUE", + "width": 3.0, + "height": 0.8, + "font_size": 18, + "wait_word": 1, + }, + { + "label": "Gamma", + "color": "C_ORANGE", + "width": 3.0, + "height": 0.8, + "font_size": 18, + "wait_word": 2, + }, + ], + } + ], + } + + +def _cascade_words() -> list[dict]: + # Starts after a short title, then 0.4s apart — 1.5s FadeIns overshoot. + return [ + {"word": "Alpha", "start": 1.2, "end": 1.4}, + {"word": "Beta", "start": 1.6, "end": 1.8}, + {"word": "Gamma", "start": 2.0, "end": 2.2}, + {"word": "tail", "start": 40.0, "end": 40.5}, + ] + + +def test_unclamped_timeline_skips_waits_when_fadein_overshoots() -> None: + """Issue #66: long FadeIn run_times push _clock past the next word start.""" + events = simulate_reveal_timeline( + _cascade_spec(), _cascade_words(), clamp_run_times=False + ) + skipped = [e for e in events if e.wait_skipped] + assert len(skipped) >= 2 + issues = reveal_cadence_violations(events, audio_end=40.5, max_skip_ratio=0.2) + assert issues, "unclamped cascade must be flagged" + + +def test_clamped_timeline_keeps_waits_aligned() -> None: + """Compile-time clamp must stop creating skip cascades.""" + events = simulate_reveal_timeline( + _cascade_spec(), _cascade_words(), clamp_run_times=True + ) + # First wait may still no-op if word_start <= title write; later ones must wait. + assert events[0].run_time <= 1.5 + assert all(e.run_time >= MIN_REVEAL_RUN_TIME for e in events) + # After clamp, Beta/Gamma should not arrive with clock already past their starts + # because prior FadeIns were shrunk. + assert not events[1].wait_skipped + assert not events[2].wait_skipped + assert reveal_cadence_violations(events, audio_end=40.5) == [] + + +def test_compile_with_words_emits_clamped_runtimes_not_garbage() -> None: + """Creation path: compile_scene_class(..., words=) must emit shortened FadeIns.""" + words = _cascade_words() + out = compile_scene_class(_cascade_spec(), words=words) + assert f"run_time={TITLE_WRITE_RUN_TIME}" in out + # Unclamped authored run_time 1.5 must not appear for every FadeIn. + assert out.count("run_time=1.5") < 3 + assert "FadeIn(_bx_0_0_0)" in out + assert "wait_until_word(timing_words, 0)" in out + assert "wait_until_word(timing_words, 1)" in out