feat(previews): isolate chapter renders and export layout bounds - #175
Open
DonIsmaelito wants to merge 21 commits into
Open
DonIsmaelito wants to merge 21 commits into
DonIsmaelito wants to merge 21 commits into
Conversation
…ames in the skill contract
…ls in order and clear the footer margin
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 23:14
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
31 issues found across 48 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_manim_teaching.py">
<violation number="1" location="tests/test_manim_teaching.py:131">
P2: The linked-value test does not verify the documented callback order because the set comparison discards ordering. Assert the ordered list of callbacks directly so an updater-order regression is detected.</violation>
</file>
<file name="AGENTS.md">
<violation number="1" location="AGENTS.md:34">
P2: The provider-independent `helpers/` contract contradicts the existing ElevenLabs-specific transcription helpers. Either move provider integrations behind an explicit adapter boundary or narrow this statement so contributors do not place or assume provider-specific code is provider-independent.</violation>
<violation number="2" location="AGENTS.md:82">
P2: This repository-wide rule sends companion-skill documentation to the wrong directory. Keep root-skill procedures in `references/`, but place companion-skill procedures in that skill's own `references/` directory so its existing relative links remain valid.</violation>
</file>
<file name="skills/manim-video/references/mobjects.md">
<violation number="1" location="skills/manim-video/references/mobjects.md:110">
P2: This example raises `NameError` because `label` is never defined in the surrounding snippet. Define the text object as `label` before grouping it, or fade out `labeled_shape` directly.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:72">
P2: Following this optional setup does not install or check LaTeX, a hard prerequisite for the documented Manim explainer workflow. Add the platform-specific LaTeX prerequisite or point users to the Manim setup instructions before claiming the feature is set up.</violation>
</file>
<file name="skills/manim-video/references/concept-explainer.md">
<violation number="1" location="skills/manim-video/references/concept-explainer.md:49">
P2: The hard 150–185 WPM range conflicts with the instruction to begin near 145 WPM when the fixed voice rate is unknown. Keep the fallback inside the stated range, such as 150 WPM.</violation>
</file>
<file name="tests/test_manim_concept_asset.py">
<violation number="1" location="tests/test_manim_concept_asset.py:46">
P2: The `None` object is masked by `width=inf`, which fails before `measured_layout_frame` visits the object. Split this into a valid-dimension `None` case and a separate infinite-width case using a valid mobject so both validation paths are covered.</violation>
</file>
<file name="SKILL.md">
<violation number="1" location="SKILL.md:210">
P2: This adds procedure prose to `SKILL.md`, outside the repository’s documentation boundary, and duplicates the existing explainer contract. Move the details to the reference file and keep only a one-line pointer here to prevent the two contracts from drifting.</violation>
</file>
<file name="skills/manim-video/references/teaching-api.md">
<violation number="1" location="skills/manim-video/references/teaching-api.md:37">
P2: Several documented actions do not return animations: `apply_force()` returns a created `Arrow`, and `dequeue()` returns an item plus a `Transform`. Qualify this statement so authors do not pass those results directly to `self.play()`.</violation>
</file>
<file name="skills/manim-video/assets/domains/_common.py">
<violation number="1" location="skills/manim-video/assets/domains/_common.py:111">
P2: When `start` or `end` is a Mobject, this helper draws from its center, so arrows overlap the source and destination objects. Compute boundary points along the center-to-center direction before constructing the arrow.</violation>
<violation number="2" location="skills/manim-video/assets/domains/_common.py:211">
P2: When the two rows have different lengths, index-matched links are not aligned to matching columns after the rows are width-normalized. Reject unequal row lengths before creating links, or explicitly align the intended columns.</violation>
</file>
<file name="install.md">
<violation number="1" location="install.md:135">
P2: On a fresh install, this new verification command fails because `pytest` is not installed by either documented dependency path, and the test suite does not import every helper as claimed. Run pytest through `uv` with a temporary pytest dependency or install it in the pip fallback, and describe the check as the test suite rather than an all-helper import check.</violation>
<violation number="2" location="install.md:135">
P2: A clean installation reaches this new verification step without `pytest`, so `python -m pytest -q` fails before testing any helper. Add pytest to a declared test dependency or install it before running this command.</violation>
</file>
<file name="skills/manim-video/assets/domains/systems.py">
<violation number="1" location="skills/manim-video/assets/domains/systems.py:75">
P2: When a long stage row triggers frame fitting, advancing the request uses an unscaled `UP * 0.30`, so the dot jumps to a different height above the target than it had at construction. Preserve the post-fit offset or derive it proportionally from the scaled stage height; apply the same correction to `DataPipeline.propagate`.</violation>
<violation number="2" location="skills/manim-video/assets/domains/systems.py:215">
P2: When a queue is moved or is large enough to trigger frame fitting, `enqueue` transforms `slots` to a fresh origin-sized group, separating it from the markers and label and potentially overflowing the frame. Build each replacement at the current slots center and scale before transforming, and apply the same fix in `dequeue`.</violation>
</file>
<file name="skills/manim-video/assets/domains/computing.py">
<violation number="1" location="skills/manim-video/assets/domains/computing.py:303">
P2: When `mix_context` is called before any links exist, `context_links` is empty but `links` contains arrows, so the first transform cannot render the attention links. Populate or replace the registered group before animating it, then fade the new links in.</violation>
</file>
<file name="skills/manim-video/assets/domains/biology.py">
<violation number="1" location="skills/manim-video/assets/domains/biology.py:138">
P2: When a long sequence triggers `frame_safe`, both replacement rows ignore the fitted scale, so transcription or translation expands outside the frame and stops aligning with DNA. Apply the original fitted scale to both replacement rows before transforming them.</violation>
<violation number="2" location="skills/manim-video/assets/domains/biology.py:206">
P2: When `PopulationFlow` was frame-fitted at construction, `transfer` replaces its scaled compartments with unscaled geometry, causing the boxes to grow outside the frame and detach from the links. Scale the replacement by the component’s fitted scale before `Transform`.</violation>
</file>
<file name="skills/manim-video/assets/domains/finance.py">
<violation number="1" location="skills/manim-video/assets/domains/finance.py:92">
P2: When a finance component is large enough for `frame_safe` to shrink it, a state-changing animation rebuilds its target at the original size and breaks the frame fit and alignment. Scale and reposition each replacement target to the current registered part before returning the transform.</violation>
<violation number="2" location="skills/manim-video/assets/domains/finance.py:183">
P2: When a caller supplies a non-finite financial value, these checks accept it and produce invalid geometry or poison later balances. Reject non-finite amounts, principals, rates, and counts before constructing the components and in transfer validation.</violation>
<violation number="3" location="skills/manim-video/assets/domains/finance.py:241">
P2: When `FeedbackLoop` has 10 or more factors, the radius cap collapses adjacent arrows and nodes. Let the radius continue growing with the factor count, or reject lists that cannot fit the ring without overlap.</violation>
</file>
<file name="skills/manim-video/SKILL.md">
<violation number="1" location="skills/manim-video/SKILL.md:3">
P2: This update changes broad SKILL.md procedure and creative-direction prose outside the allowed edit scope in `AGENTS.md`. Move that procedure into the referenced documentation files and limit SKILL.md to the permitted contract changes.</violation>
<violation number="2" location="skills/manim-video/SKILL.md:206">
P2: After the documented `-qh` render, these concat paths point at the low-quality draft outputs. Use the production output directory, or make the stitch example explicitly consume a preceding `-ql` render.</violation>
</file>
<file name="tests/test_comment_style.py">
<violation number="1" location="tests/test_comment_style.py:15">
P2: Comments containing `!`, `?`, `%`, hyphens, and other punctuation pass this guard because the regex only lists nine characters. Broaden the check to cover the full punctuation set so newly added definitions cannot bypass the convention.</violation>
</file>
<file name="skills/manim-video/assets/domains/math.py">
<violation number="1" location="skills/manim-video/assets/domains/math.py:179">
P2: When `MatrixMap` receives a non-identity matrix, the initial output arrow still matches the input arrow. Compute its endpoint using the default input vector and `values` during construction.</violation>
<violation number="2" location="skills/manim-video/assets/domains/math.py:323">
P2: When `amount` is NaN, `ensure_amount()` accepts it and corrupts both probability entries. Reject non-finite amounts before mutating `self.probabilities`.</violation>
</file>
<file name="skills/manim-video/assets/domains/physics.py">
<violation number="1" location="skills/manim-video/assets/domains/physics.py:81">
P2: When `move_body` runs, only the body moves while `connections` stays at its construction-time endpoints. Animate the affected connection lines with the body so the system remains joined.</violation>
<violation number="2" location="skills/manim-video/assets/domains/physics.py:139">
P2: After `set_vector` changes the arrow, the force label remains at the previous endpoint. Animate `part("label")` to the new arrow tip as part of the same update.</violation>
<violation number="3" location="skills/manim-video/assets/domains/physics.py:265">
P2: After several `flow` calls, charges leave the top wire instead of circulating around the loop. Wrap each charge to the segment or animate it along the circuit path.</violation>
</file>
<file name="skills/manim-video/assets/teaching.py">
<violation number="1" location="skills/manim-video/assets/teaching.py:284">
P2: After `transform_object` adopts a semantic target with callable anchors, the anchors become fixed at the transform position. Preserve callables tied to the adopted children or rebind them structurally so anchors continue following later movement.</violation>
<violation number="2" location="skills/manim-video/assets/teaching.py:473">
P2: When `focus_on` receives a nested part, it drops the entire containing scene root, so unrelated sibling parts remain fully visible. Dim unrelated descendants instead of excluding any root that contains the target.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| # FadeOut everything on screen (may contain mixed types) | ||
| self.play(FadeOut(Group(*self.mobjects))) | ||
| # A deliberate mixed-type departure can use Group. | ||
| self.play(FadeOut(Group(label, circle))) |
There was a problem hiding this comment.
P2: This example raises NameError because label is never defined in the surrounding snippet. Define the text object as label before grouping it, or fade out labeled_shape directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/references/mobjects.md, line 110:
<comment>This example raises `NameError` because `label` is never defined in the surrounding snippet. Define the text object as `label` before grouping it, or fade out `labeled_shape` directly.</comment>
<file context>
@@ -106,8 +106,8 @@ shapes.set_color(BLUE)
-# FadeOut everything on screen (may contain mixed types)
-self.play(FadeOut(Group(*self.mobjects)))
+# A deliberate mixed-type departure can use Group.
+self.play(FadeOut(Group(label, circle)))
</file context>
</details>
```suggestion
self.play(FadeOut(labeled_shape))
| cat > concat.txt << 'EOF' | ||
| file 'media/videos/script/480p15/Scene1_Introduction.mp4' | ||
| file 'media/videos/script/480p15/Scene2_CoreConcept.mp4' | ||
| file 'media/videos/script/480p15/CompositionChapter.mp4' |
There was a problem hiding this comment.
P2: After the documented -qh render, these concat paths point at the low-quality draft outputs. Use the production output directory, or make the stitch example explicitly consume a preceding -ql render.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/SKILL.md, line 206:
<comment>After the documented `-qh` render, these concat paths point at the low-quality draft outputs. Use the production output directory, or make the stitch example explicitly consume a preceding `-ql` render.</comment>
<file context>
@@ -108,84 +123,104 @@ project-name/
cat > concat.txt << 'EOF'
-file 'media/videos/script/480p15/Scene1_Introduction.mp4'
-file 'media/videos/script/480p15/Scene2_CoreConcept.mp4'
+file 'media/videos/script/480p15/CompositionChapter.mp4'
+file 'media/videos/script/480p15/SystemsChapter.mp4'
EOF
</file context>
Suggested change
| file 'media/videos/script/480p15/CompositionChapter.mp4' | |
| file 'media/videos/script/1080p60/CompositionChapter.mp4' |
|
|
||
| ROOT = Path(__file__).resolve().parents[1] | ||
| SKIP_DIRS = {".venv", "venv", "node_modules", "__pycache__", ".git", "media", "edit"} | ||
| PUNCTUATION = re.compile(r"[.,:;()\"'`]") |
There was a problem hiding this comment.
P2: Comments containing !, ?, %, hyphens, and other punctuation pass this guard because the regex only lists nine characters. Broaden the check to cover the full punctuation set so newly added definitions cannot bypass the convention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_comment_style.py, line 15:
<comment>Comments containing `!`, `?`, `%`, hyphens, and other punctuation pass this guard because the regex only lists nine characters. Broaden the check to cover the full punctuation set so newly added definitions cannot bypass the convention.</comment>
<file context>
@@ -0,0 +1,83 @@
+
+ROOT = Path(__file__).resolve().parents[1]
+SKIP_DIRS = {".venv", "venv", "node_modules", "__pycache__", ".git", "media", "edit"}
+PUNCTUATION = re.compile(r"[.,:;()\"'`]")
+DEFINITIONS = (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)
+
</file context>
Suggested change
| PUNCTUATION = re.compile(r"[.,:;()\"'`]") | |
| PUNCTUATION = re.compile(r"[^\w\s]") |
| ```bash | ||
| python ~/Developer/video-use/helpers/timeline_view.py --help >/dev/null && echo "helpers OK" | ||
| ffprobe -version | head -1 | ||
| cd ~/Developer/video-use && python -m pytest -q # proves every helper imports; Manim tests skip when it is not installed |
There was a problem hiding this comment.
P2: A clean installation reaches this new verification step without pytest, so python -m pytest -q fails before testing any helper. Add pytest to a declared test dependency or install it before running this command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install.md, line 135:
<comment>A clean installation reaches this new verification step without `pytest`, so `python -m pytest -q` fails before testing any helper. Add pytest to a declared test dependency or install it before running this command.</comment>
<file context>
@@ -132,6 +132,7 @@ Run one real thing. Prefer the lightest verification that still proves the pipel
```bash
python ~/Developer/video-use/helpers/timeline_view.py --help >/dev/null && echo "helpers OK"
ffprobe -version | head -1
+cd ~/Developer/video-use && python -m pytest -q # proves every helper imports; Manim tests skip when it is not installed
</file context>
</details>
<a href="https://www.cubic.dev/action/fix/violation/4225f562-55d7-4177-88b5-b43dcf0d715b" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true">
<picture>
<source media="(prefers-color-scheme: dark)" srcset="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
<source media="(prefers-color-scheme: light)" srcset="https://cubic.dev/buttons/fix-with-cubic-light.svg">
<img alt="Fix with cubic" src="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
</picture>
</a>
Comment on lines
+82
to
+85
| - Procedure prose belongs in `references/<feature>.md` at the repository root; | ||
| create that folder with the first reference file. Edits to `SKILL.md` | ||
| are limited to rules, helper-index bullets, directory-tree lines, the EDL | ||
| example, and one-line pointers to the reference files. |
There was a problem hiding this comment.
P2: This repository-wide rule sends companion-skill documentation to the wrong directory. Keep root-skill procedures in references/, but place companion-skill procedures in that skill's own references/ directory so its existing relative links remain valid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At AGENTS.md, line 82:
<comment>This repository-wide rule sends companion-skill documentation to the wrong directory. Keep root-skill procedures in `references/`, but place companion-skill procedures in that skill's own `references/` directory so its existing relative links remain valid.</comment>
<file context>
@@ -0,0 +1,87 @@
+ history, so a later sync silently discards it.
+- Hard rules in `SKILL.md` are append-only. New rules get the next number.
+ Removing or renumbering a rule requires an explicit reason in the commit.
+- Procedure prose belongs in `references/<feature>.md` at the repository root;
+ create that folder with the first reference file. Edits to `SKILL.md`
+ are limited to rules, helper-index bullets, directory-tree lines, the EDL
</file context>
Suggested change
| - Procedure prose belongs in `references/<feature>.md` at the repository root; | |
| create that folder with the first reference file. Edits to `SKILL.md` | |
| are limited to rules, helper-index bullets, directory-tree lines, the EDL | |
| example, and one-line pointers to the reference files. | |
| - Procedure prose for the root skill belongs in `references/<feature>.md`; companion-skill prose belongs in that skill's `references/` directory. | |
| Create the relevant directory with the first reference file. Edits to `SKILL.md` are limited to rules, helper-index bullets, directory-tree lines, the EDL example, and one-line pointers to reference files. |
| ## Architecture boundaries | ||
|
|
||
| - `SKILL.md` defines the agent workflow and public editing contract. | ||
| - `helpers/` contains provider-independent production tools and validation. |
There was a problem hiding this comment.
P2: The provider-independent helpers/ contract contradicts the existing ElevenLabs-specific transcription helpers. Either move provider integrations behind an explicit adapter boundary or narrow this statement so contributors do not place or assume provider-specific code is provider-independent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At AGENTS.md, line 34:
<comment>The provider-independent `helpers/` contract contradicts the existing ElevenLabs-specific transcription helpers. Either move provider integrations behind an explicit adapter boundary or narrow this statement so contributors do not place or assume provider-specific code is provider-independent.</comment>
<file context>
@@ -0,0 +1,87 @@
+## Architecture boundaries
+
+- `SKILL.md` defines the agent workflow and public editing contract.
+- `helpers/` contains provider-independent production tools and validation.
+- `skills/` contains focused companion skills and reusable production assets.
+- `tests/` protects public behavior. Optional clients or remote runners may
</file context>
Suggested change
| - `helpers/` contains provider-independent production tools and validation. | |
| - `helpers/` contains reusable production tools and narrow provider adapters. |
Author
|
@cubic-dev-ai please review the latest commits again. Run preview renders from the project root and include the concept helper in copy instructions. Inherits the #146 camera and numeric fixes. Validation: 136 branch tests passed. The combined core preview passed 765 tests with two optional skips. Existing threads remain open for rechecking; this follow-up does not claim every previous finding is resolved. |
@DonIsmaelito I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 existing issues remain and 10 new issues found across 48 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/manim-video/assets/domains/math.py">
<violation number="1" location="skills/manim-video/assets/domains/math.py:115">
P2: When `apply_matrix` receives a 2×2 matrix containing `NaN` or infinity, it passes non-finite coordinates into Manim and can produce an invalid render. Reject non-finite matrix entries alongside the shape check.</violation>
<violation number="2" location="skills/manim-video/assets/domains/math.py:285">
P2: `ProbabilityMass` accepts distributions that do not sum to one because the default relative tolerance is much larger than the explicit absolute tolerance. Set `rtol=0` so only the intended `1e-8` absolute tolerance applies.</violation>
</file>
<file name="skills/manim-video/references/teaching-api.md">
<violation number="1" location="skills/manim-video/references/teaching-api.md:3">
P3: The copy instruction omits `assets/domains/_common.py` and the package layout, so a literal follow breaks the imports it recommends. Every `assets/domains/` module starts with `from ._common import ...` (e.g. `math.py:30`), and `_common.py` itself imports `from ..teaching import ...`; the copied `assets/__init__.py` is what makes `from domains import MatrixMap` work in the SKILL.md example. A user copying only `teaching.py`, `concept_explainer.py`, and e.g. `math.py` hits `ModuleNotFoundError`, and a flat (non-package) copy fails with a relative-import error. State explicitly that `assets/domains/_common.py` (plus the `assets/__init__.py` / `domains/__init__.py` package structure) is required.</violation>
</file>
<file name="skills/manim-video/assets/concept_explainer.py">
<violation number="1" location="skills/manim-video/assets/concept_explainer.py:41">
P2: A `NaN` coordinate passes both range checks, so `normalized_region()` moves a guide to invalid coordinates instead of rejecting the input. Reject nonfinite rectangle values before the bounds checks.</violation>
<violation number="2" location="skills/manim-video/assets/concept_explainer.py:75">
P2: Passing a zero-width or zero-height Mobject, such as a horizontal `Line`, divides by zero before it can be fitted. Clamp zero dimensions to a small positive value or treat them as unbounded.</violation>
<violation number="3" location="skills/manim-video/assets/concept_explainer.py:156">
P1: Calling `assert_inside_frame()`, `normalized_bounds()`, or `source_footer()` raises `NameError` because this file never imports `np`. Add `import numpy as np` before these helpers use it.</violation>
</file>
<file name="tests/test_preview_scene.py">
<violation number="1" location="tests/test_preview_scene.py:51">
P2: These option-validation assertions never exercise the validation path. `tmp_path / 'absent.py'` does not exist, so if the fps/quality/timeout checks were deleted the call would fail later with `Manim script does not exist: ...` — still a `PreviewError`, so `pytest.raises(preview_scene.PreviewError)` still passes. The test's promise ("reject invalid clocks before invoking any executable") is not actually protected. Create a valid scene script first so the only remaining raise is the option validation, and ideally match each option-specific message.</violation>
</file>
<file name="tests/test_manim_domains.py">
<violation number="1" location="tests/test_manim_domains.py:34">
P3: The `0.9` bound duplicates the `2 * margin` (0.45 per side) that `fit_to_frame` in domains/_common.py applies during `frame_safe`. If that default margin ever changes, this assertion silently stops matching the actual fit guarantee. Derive the bound from the production margin constant instead of the magic number.</violation>
</file>
<file name="skills/manim-video/scripts/preview_scene.py">
<violation number="1" location="skills/manim-video/scripts/preview_scene.py:276">
P3: When the sections JSON is unreadable, `_section_metadata` returns a synthetic entry shaped `{metadata_file, status}` that is not a section dict, while the success path returns raw Manim section dicts with `name`, `playback_time`, and `type`. Any consumer iterating `report["sections"]` (e.g. to map chapter names to timestamps) hits a `KeyError` on the fallback entry. Return a section-shaped entry or omit the placeholder so the schema stays consistent.</violation>
<violation number="2" location="skills/manim-video/scripts/preview_scene.py:319">
P3: Every attempt renders into a fresh `--media_dir` and the previous `attempt-*` directory, so the Manim cache never survives between runs. This contradicts the skill docs, which state the preview command "uses Manim Community low quality and its cache" (SKILL.md Step 5 and references/concept-explainer.md Targeted Preview). Either keep a reusable cache directory per source (it cannot validate a render that wrote nothing, since video selection already comes only from the current media dir) or update both doc sections to drop the cache claim.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| margin: float = 0.18, | ||
| ) -> Mobject: | ||
| """Raise before render if a mobject leaves the frame or caption-safe area.""" | ||
| if not np.isfinite(margin) or margin < 0 or not np.isfinite(caption_rail_fraction) or not 0 <= caption_rail_fraction < 1: |
There was a problem hiding this comment.
P1: Calling assert_inside_frame(), normalized_bounds(), or source_footer() raises NameError because this file never imports np. Add import numpy as np before these helpers use it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/assets/concept_explainer.py, line 156:
<comment>Calling `assert_inside_frame()`, `normalized_bounds()`, or `source_footer()` raises `NameError` because this file never imports `np`. Add `import numpy as np` before these helpers use it.</comment>
<file context>
@@ -0,0 +1,290 @@
+ margin: float = 0.18,
+) -> Mobject:
+ """Raise before render if a mobject leaves the frame or caption-safe area."""
+ if not np.isfinite(margin) or margin < 0 or not np.isfinite(caption_rail_fraction) or not 0 <= caption_rail_fraction < 1:
+ raise ValueError("frame margin and caption rail must be finite nonnegative bounds")
+ # compute the allowed bounds with the caption rail folded into the bottom edge
</file context>
| if not self.probabilities or any(value < 0 for value in self.probabilities): | ||
| raise ValueError("probabilities must be a non-empty list of non-negative values") | ||
| total = sum(self.probabilities) | ||
| if not np.isclose(total, 1.0, atol=1e-8): |
There was a problem hiding this comment.
P2: ProbabilityMass accepts distributions that do not sum to one because the default relative tolerance is much larger than the explicit absolute tolerance. Set rtol=0 so only the intended 1e-8 absolute tolerance applies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/assets/domains/math.py, line 285:
<comment>`ProbabilityMass` accepts distributions that do not sum to one because the default relative tolerance is much larger than the explicit absolute tolerance. Set `rtol=0` so only the intended `1e-8` absolute tolerance applies.</comment>
<file context>
@@ -0,0 +1,337 @@
+ if not self.probabilities or any(value < 0 for value in self.probabilities):
+ raise ValueError("probabilities must be a non-empty list of non-negative values")
+ total = sum(self.probabilities)
+ if not np.isclose(total, 1.0, atol=1e-8):
+ raise ValueError(f"probabilities must sum to one, got {total:g}")
+ self.labels = [str(value) for value in labels] if labels is not None else [str(i) for i in range(len(self.probabilities))]
</file context>
Suggested change
| if not np.isclose(total, 1.0, atol=1e-8): | |
| if not np.isclose(total, 1.0, atol=1e-8, rtol=0): |
|
|
||
| # multiply every vector by a two by two matrix and return the animation that redraws the arrows | ||
| def apply_matrix(self, matrix: Sequence[Sequence[float]]) -> AnimationGroup: | ||
| transform = np.asarray(matrix, dtype=float) |
There was a problem hiding this comment.
P2: When apply_matrix receives a 2×2 matrix containing NaN or infinity, it passes non-finite coordinates into Manim and can produce an invalid render. Reject non-finite matrix entries alongside the shape check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/assets/domains/math.py, line 115:
<comment>When `apply_matrix` receives a 2×2 matrix containing `NaN` or infinity, it passes non-finite coordinates into Manim and can produce an invalid render. Reject non-finite matrix entries alongside the shape check.</comment>
<file context>
@@ -0,0 +1,337 @@
+
+ # multiply every vector by a two by two matrix and return the animation that redraws the arrows
+ def apply_matrix(self, matrix: Sequence[Sequence[float]]) -> AnimationGroup:
+ transform = np.asarray(matrix, dtype=float)
+ if transform.shape != (2, 2):
+ raise ValueError("vector map transformations require a 2 by 2 matrix")
</file context>
Suggested change
| transform = np.asarray(matrix, dtype=float) | |
| transform = np.asarray(matrix, dtype=float) | |
| if transform.shape != (2, 2) or not np.all(np.isfinite(transform)): | |
| raise ValueError("vector map transformations require a finite 2 by 2 matrix") |
| x, y, width, height = (float(rect[key]) for key in ("x", "y", "width", "height")) | ||
| except (KeyError, TypeError, ValueError) as exc: | ||
| raise ValueError("region requires numeric x, y, width, and height") from exc | ||
| if x < 0 or y < 0 or width <= 0 or height <= 0: |
There was a problem hiding this comment.
P2: A NaN coordinate passes both range checks, so normalized_region() moves a guide to invalid coordinates instead of rejecting the input. Reject nonfinite rectangle values before the bounds checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/assets/concept_explainer.py, line 41:
<comment>A `NaN` coordinate passes both range checks, so `normalized_region()` moves a guide to invalid coordinates instead of rejecting the input. Reject nonfinite rectangle values before the bounds checks.</comment>
<file context>
@@ -0,0 +1,290 @@
+ x, y, width, height = (float(rect[key]) for key in ("x", "y", "width", "height"))
+ except (KeyError, TypeError, ValueError) as exc:
+ raise ValueError("region requires numeric x, y, width, and height") from exc
+ if x < 0 or y < 0 or width <= 0 or height <= 0:
+ raise ValueError("region must use non-negative x/y and positive width/height")
+ if x + width > 1.000001 or y + height > 1.000001:
</file context>
Suggested change
| if x < 0 or y < 0 or width <= 0 or height <= 0: | |
| if not all(math.isfinite(value) for value in (x, y, width, height)) or x < 0 or y < 0 or width <= 0 or height <= 0: |
| """Scale down, never up, until a mobject fits the requested box.""" | ||
| if max_width <= 0 or max_height <= 0: | ||
| raise ValueError("max_width and max_height must be positive") | ||
| scale = min(1.0, max_width / mobject.width, max_height / mobject.height) |
There was a problem hiding this comment.
P2: Passing a zero-width or zero-height Mobject, such as a horizontal Line, divides by zero before it can be fitted. Clamp zero dimensions to a small positive value or treat them as unbounded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/assets/concept_explainer.py, line 75:
<comment>Passing a zero-width or zero-height Mobject, such as a horizontal `Line`, divides by zero before it can be fitted. Clamp zero dimensions to a small positive value or treat them as unbounded.</comment>
<file context>
@@ -0,0 +1,290 @@
+ """Scale down, never up, until a mobject fits the requested box."""
+ if max_width <= 0 or max_height <= 0:
+ raise ValueError("max_width and max_height must be positive")
+ scale = min(1.0, max_width / mobject.width, max_height / mobject.height)
+ mobject.scale(scale)
+ return mobject
</file context>
Suggested change
| scale = min(1.0, max_width / mobject.width, max_height / mobject.height) | |
| scale = min(1.0, max_width / max(mobject.width, 1e-9), max_height / max(mobject.height, 1e-9)) |
| {'fps': float('inf')}, {'quality': 'ultra'}, {'timeout_s': -1}]) | ||
| def test_invalid_render_options(tmp_path, kwargs): | ||
| with pytest.raises(preview_scene.PreviewError): | ||
| preview_scene.render_scene(tmp_path / 'absent.py', 'Demo', **kwargs) |
There was a problem hiding this comment.
P2: These option-validation assertions never exercise the validation path. tmp_path / 'absent.py' does not exist, so if the fps/quality/timeout checks were deleted the call would fail later with Manim script does not exist: ... — still a PreviewError, so pytest.raises(preview_scene.PreviewError) still passes. The test's promise ("reject invalid clocks before invoking any executable") is not actually protected. Create a valid scene script first so the only remaining raise is the option validation, and ideally match each option-specific message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_preview_scene.py, line 51:
<comment>These option-validation assertions never exercise the validation path. `tmp_path / 'absent.py'` does not exist, so if the fps/quality/timeout checks were deleted the call would fail later with `Manim script does not exist: ...` — still a `PreviewError`, so `pytest.raises(preview_scene.PreviewError)` still passes. The test's promise ("reject invalid clocks before invoking any executable") is not actually protected. Create a valid scene script first so the only remaining raise is the option validation, and ideally match each option-specific message.</comment>
<file context>
@@ -0,0 +1,235 @@
+ {'fps': float('inf')}, {'quality': 'ultra'}, {'timeout_s': -1}])
+def test_invalid_render_options(tmp_path, kwargs):
+ with pytest.raises(preview_scene.PreviewError):
+ preview_scene.render_scene(tmp_path / 'absent.py', 'Demo', **kwargs)
+
+
</file context>
Comment on lines
+3
to
+4
| Copy `assets/teaching.py`, `assets/concept_explainer.py`, and the needed `assets/domains/` modules into an | ||
| original explainer workspace. Pass one shared `VisualTheme` to every component. |
There was a problem hiding this comment.
P3: The copy instruction omits assets/domains/_common.py and the package layout, so a literal follow breaks the imports it recommends. Every assets/domains/ module starts with from ._common import ... (e.g. math.py:30), and _common.py itself imports from ..teaching import ...; the copied assets/__init__.py is what makes from domains import MatrixMap work in the SKILL.md example. A user copying only teaching.py, concept_explainer.py, and e.g. math.py hits ModuleNotFoundError, and a flat (non-package) copy fails with a relative-import error. State explicitly that assets/domains/_common.py (plus the assets/__init__.py / domains/__init__.py package structure) is required.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/references/teaching-api.md, line 3:
<comment>The copy instruction omits `assets/domains/_common.py` and the package layout, so a literal follow breaks the imports it recommends. Every `assets/domains/` module starts with `from ._common import ...` (e.g. `math.py:30`), and `_common.py` itself imports `from ..teaching import ...`; the copied `assets/__init__.py` is what makes `from domains import MatrixMap` work in the SKILL.md example. A user copying only `teaching.py`, `concept_explainer.py`, and e.g. `math.py` hits `ModuleNotFoundError`, and a flat (non-package) copy fails with a relative-import error. State explicitly that `assets/domains/_common.py` (plus the `assets/__init__.py` / `domains/__init__.py` package structure) is required.</comment>
<file context>
@@ -0,0 +1,60 @@
+# Semantic Teaching API
+
+Copy `assets/teaching.py`, `assets/concept_explainer.py`, and the needed `assets/domains/` modules into an
+original explainer workspace. Pass one shared `VisualTheme` to every component.
+These APIs manage semantic identity, linked state, attention, and continuity;
</file context>
Suggested change
| Copy `assets/teaching.py`, `assets/concept_explainer.py`, and the needed `assets/domains/` modules into an | |
| original explainer workspace. Pass one shared `VisualTheme` to every component. | |
| Copy `assets/teaching.py`, `assets/concept_explainer.py`, the `assets/domains/_common.py` helper, and the chosen `assets/domains/` modules (keeping the `assets/` and `assets/domains/` package layout with their `__init__.py` files) into an | |
| original explainer workspace. |
|
|
||
| assert isinstance(model, SemanticMobject) | ||
| assert model.part_names | ||
| assert model.width <= float(manim.config.frame_width) - 0.9 + 1e-6 |
There was a problem hiding this comment.
P3: The 0.9 bound duplicates the 2 * margin (0.45 per side) that fit_to_frame in domains/_common.py applies during frame_safe. If that default margin ever changes, this assertion silently stops matching the actual fit guarantee. Derive the bound from the production margin constant instead of the magic number.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_manim_domains.py, line 34:
<comment>The `0.9` bound duplicates the `2 * margin` (0.45 per side) that `fit_to_frame` in domains/_common.py applies during `frame_safe`. If that default margin ever changes, this assertion silently stops matching the actual fit guarantee. Derive the bound from the production margin constant instead of the magic number.</comment>
<file context>
@@ -0,0 +1,88 @@
+
+ assert isinstance(model, SemanticMobject)
+ assert model.part_names
+ assert model.width <= float(manim.config.frame_width) - 0.9 + 1e-6
+ assert model.height <= float(manim.config.frame_height) - 0.9 + 1e-6
+
</file context>
| ffprobe = require_executable(ffprobe_bin, purpose="ffprobe") | ||
| scene_root.mkdir(parents=True, exist_ok=True) | ||
| # isolate every attempt so older videos cannot validate a render that wrote nothing | ||
| destination = Path(tempfile.mkdtemp(prefix="attempt-", dir=scene_root)) |
There was a problem hiding this comment.
P3: Every attempt renders into a fresh --media_dir and the previous attempt-* directory, so the Manim cache never survives between runs. This contradicts the skill docs, which state the preview command "uses Manim Community low quality and its cache" (SKILL.md Step 5 and references/concept-explainer.md Targeted Preview). Either keep a reusable cache directory per source (it cannot validate a render that wrote nothing, since video selection already comes only from the current media dir) or update both doc sections to drop the cache claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/scripts/preview_scene.py, line 319:
<comment>Every attempt renders into a fresh `--media_dir` and the previous `attempt-*` directory, so the Manim cache never survives between runs. This contradicts the skill docs, which state the preview command "uses Manim Community low quality and its cache" (SKILL.md Step 5 and references/concept-explainer.md Targeted Preview). Either keep a reusable cache directory per source (it cannot validate a render that wrote nothing, since video selection already comes only from the current media dir) or update both doc sections to drop the cache claim.</comment>
<file context>
@@ -0,0 +1,427 @@
+ ffprobe = require_executable(ffprobe_bin, purpose="ffprobe")
+ scene_root.mkdir(parents=True, exist_ok=True)
+ # isolate every attempt so older videos cannot validate a render that wrote nothing
+ destination = Path(tempfile.mkdtemp(prefix="attempt-", dir=scene_root))
+ media_dir = destination / "media"
+ command = [
</file context>
| try: | ||
| data = json.loads(source.read_text(encoding="utf-8")) | ||
| except (OSError, json.JSONDecodeError): | ||
| return [{"metadata_file": str(source), "status": "unreadable"}] |
There was a problem hiding this comment.
P3: When the sections JSON is unreadable, _section_metadata returns a synthetic entry shaped {metadata_file, status} that is not a section dict, while the success path returns raw Manim section dicts with name, playback_time, and type. Any consumer iterating report["sections"] (e.g. to map chapter names to timestamps) hits a KeyError on the fallback entry. Return a section-shaped entry or omit the placeholder so the schema stays consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/manim-video/scripts/preview_scene.py, line 276:
<comment>When the sections JSON is unreadable, `_section_metadata` returns a synthetic entry shaped `{metadata_file, status}` that is not a section dict, while the success path returns raw Manim section dicts with `name`, `playback_time`, and `type`. Any consumer iterating `report["sections"]` (e.g. to map chapter names to timestamps) hits a `KeyError` on the fallback entry. Return a section-shaped entry or omit the placeholder so the schema stays consistent.</comment>
<file context>
@@ -0,0 +1,427 @@
+ try:
+ data = json.loads(source.read_text(encoding="utf-8"))
+ except (OSError, json.JSONDecodeError):
+ return [{"metadata_file": str(source), "status": "unreadable"}]
+ sections = data if isinstance(data, list) else data.get("sections", [])
+ return [dict(section) for section in sections if isinstance(section, dict)]
</file context>
Suggested change
| return [{"metadata_file": str(source), "status": "unreadable"}] | |
| return [{"name": None, "type": "unreadable", "playback_time": 0.0, "metadata_file": str(source), "status": "unreadable"}] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A failed chapter preview should not accidentally reuse an old video. Layout review also needs measurements that show when objects extend outside the frame.
Use this for Manim-based math, science and technical explainers that need chapter-by-chapter checks of animated diagrams, equations and text layouts.
Builds on #146.
Changes
Isolate each preview attempt by source path and a fresh output directory.
Expose FPS and quality controls, validate options, and reuse already decoded endpoint frames in contact sheets.
Export named object bounds in delivery pixels, including clipping and unrotated camera movement.
Feature commits and review fixes cover preview reliability and measured layout evidence. All 136 branch tests pass, including real Manim rendering.
Review follow-up: Run preview renders from the project root and include the concept helper in copy instructions. Inherits the manim teaching assets, semantic chapters, and chapter preview tooling #146 camera and numeric fixes.
Limits
Measured rectangles are evidence, not automatic layout approval. Rotated or perspective cameras and internal labels need separate review. Failed attempts remain on disk, and source path hashes are not content fingerprints or a render cache.
This is an incremental follow-up to #146. Measurements can feed the layout checks in #148 without requiring that PR to export them. Broader teaching helpers and workflow changes remain outside this draft.