chantier: Remotion gaps — the S-effort wave (5 workstreams) - #168
Merged
Conversation
…ered `rustmotion skills install` is the only channel through which a generating agent receives the rules. `SKILL_FILES` embedded 30 of the 47 on disk. The 17 absentees were not a random sample. They included `geometry-safety.md`, `world-view.md` and `audio-reactive.md` — the three CLAUDE.md cites by name — and `html-css-mental-model.md`, which PR #156 had just corrected because it taught JSON forms that delete the component from the video. That file had never been in the list, before or after the fix: the content was repaired and the pipe was not. The list is now generated by a build script that walks the skills tree, so "every rule is embedded" holds by construction rather than by memory. Adding 17 entries by hand would have left the next rule just as invisible. Two details the generation had to preserve: the table is still ordered with SKILL.md first, and `show()` no longer depends on that — it locates SKILL.md by path, where it previously indexed position 0. Verified by running the installed binary into an empty directory: 49 files written, 47 of them rules. A test now compares what is embedded against the directory and fails naming any file that drifts out.
…eroing it A `font-size` in a relative unit resolved to 0px without failing. `validate` returned exit 0 and "Valid scenario" on a `text` with `"font-size": "2rem"`, warning only that it was "resolving to 0px" — and the render produced a frame with no text in it. The contextual accessor already existed and was tested; nothing called it. Every component reached for `font_size_px_or`, the context-free one. The resolution pattern was already written and proven in `text.rs` for `letter-spacing` and `line-height`: build a length context locally from the frame dimensions. This extends it to `font-size` itself and rolls it out across the components that render text, through one shared `intrinsic::font_size_ctx` rather than fifteen copies — copy-paste of a derived value is what produced several of the defects this chantier is fixing. `PaintCtx` is deliberately untouched: threading the context through it would have rippled into all 57 painters. Each component has a test that paints and asserts non-transparent pixels, not merely that a number came back non-zero. Measurement and paint were checked together, since the geometry validator reads the measured box: a component that measured 0 while painting 32px would have moved the bug rather than fixed it. Known limit, unchanged: `em` and `%` on `font-size` depend on the parent's resolved size, and the cascade still copies the parent's raw unresolved `Length`. `rem`, `vw` and `vh` are correct now; `em` and `%` remain approximate.
…ible Three defects that all end the same way: a hole in the video and no diagnostic. - Icon preloading could never work. The painter keyed its cache on the oversampled render size, the preloader on the target size — for a 40x40 icon one wrote `80x80` and the other read `40x40`, so the keys could not collide at any size. The mechanism meant to stop N render threads hitting the network in parallel added a wasted download and rasterization, then let every thread fetch anyway. The preloader also rasterized without the oversample, so fixing the key alone would have turned a useless preload into a harmful one: the painter would finally find a bitmap, at half the resolution. Both sites now go through one `icon_cache_key`, which applies the oversample and builds the key — the drift is no longer expressible. - Icons had no disk cache and failed silently at every level. There is now a cache on disk, following the pattern `google_fonts.rs` already uses in this crate, and an unresolvable icon fails the preload instead of leaving a gap. - Video frame extraction failed silently too: a missing or failing ffmpeg produced an entirely empty video with no warning. PR #151 had already hardened the *audio* extraction of embedded videos in this same crate, with an availability probe and a warning emitted once; that discipline is now extended to the frame path, which had been left behind. Warnings added here are deduplicated: `prepare_scene` runs per frame, so an unguarded one would print over a thousand times on a 1200-frame render.
…lable `ffmpeg_args` knew only software encoders. On machines that have VideoToolbox or NVENC, the encode step was leaving an order of magnitude on the table. `--hardware-acceleration` now probes `ffmpeg -encoders` for what the machine actually offers rather than inferring it from the compile target: a macOS box without a usable VideoToolbox exists, and so does an ffmpeg built without NVENC. When acceleration is asked for and unavailable, it says so and continues in software — it neither aborts nor switches silently. CRF is meaningless on most hardware encoders, which think in bitrate, so `check_crf` now warns explicitly when `--crf` is passed alongside an acceleration that ignores it, instead of letting the setting look honoured. PR #143 had extracted `ffmpeg_args` into a pure function after an argument ordering bug put an input after the output options. That property is preserved and is why this was cheap: the probe (machine-dependent) and the argument construction (pure) are separate, and the tests assert the presence *and position* of the hardware arguments without needing a machine that has one — CI does not. `cmd_render` grew a parameter, so `cmd_batch`'s call site passes `false`: `batch` has no flag of its own, and wiring one is a separate change.
… settles A spring's duration used to be an emergent consequence of `damping`, `stiffness` and `mass`. An author who wanted "this spring, finishing in 0.8s" had to guess at three coupled numbers, and nothing exposed the instant a given spring actually comes to rest — the one figure you need to time the rest of a scene against it. `SpringConfig` now takes an optional `duration` and `rest_threshold`. When `duration` is set, time is rescaled linearly so the settle point lands exactly there. The spring's shape is preserved by construction — the solver is called with a scaled `t`, not with altered physics — and a test asserts it: sampled at matching fractions of each one's own settle time, the natural and pinned curves agree to 1e-9, with identical oscillation counts and overshoot amplitudes. Without that test nothing would distinguish a real remap from a crude shortening that flattens the spring. The settle search handles the regimes that break naive implementations: an overdamped spring never reaches its target exactly, which is what `rest_threshold` is for; a very lightly damped one can take arbitrarily long, so the search is bounded and the bound is tested rather than left to spin. `rustmotion info` now reports each spring's rest time, so a scene can be timed against it instead of by trial and error, and `validate` rejects a non-positive `duration` or a negative `rest_threshold` alongside the checks PR #158 already added.
LeadcodeDev
changed the title
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.
Follows the audit chantier (#142, merged by #163) and the re-scoring of the Remotion differential. Tracking: issue #167.
The five S-effort gaps at the top of the re-scored list, plus two defects found while verifying them.
font-sizein relative unitsThe two corrections that matter most
Workstream A repairs a pipe, not a payload.
rustmotion skills installis the only channel through which a generating agent receives the rules. It distributed 30 of 47. The absentees included the three CLAUDE.md cites by name andhtml-css-mental-model.md, which PR #156 had just corrected because it taught JSON forms that delete the component from the video. That file had never been in the list: the fix reached nobody.The literal list is deleted, not completed: a
build.rswalks the rules tree, making "every rule is embedded" true by construction. Adding the 17 by hand would have left the next rule just as invisible. Verified by running the binary into an empty directory: 49 files written, 47 of them rules.Workstream B closes a silent black frame. A
font-sizein a relative unit resolved to 0px without failing:validatereturned exit 0 and "Valid scenario", and the render did not contain the text. The contextual accessor existed and was tested — nothing called it.Every component now has a test that paints and asserts non-transparent pixels, not merely that a number came back non-zero. Measurement and paint were checked together, since the geometry pass reads the measured box: a component measuring 0 while painting 32px would have moved the bug rather than fixed it.
What workstream C uncovered
Icon preloading could not work: the painter keyed its cache on the oversampled render size, the preloader on the target size — for a 40x40 icon one wrote
80x80and the other read40x40. The mechanism meant to avoid concurrent network requests added one per thread, after wasting a download.The trap avoided: fixing the key without fixing the rasterization would have turned a useless preload into a harmful one — the painter would finally find a bitmap, at half resolution. Both sites now go through a single
icon_cache_key; the drift is no longer expressible.Verification — a difference in rigour worth stating
Unlike the previous chantier, the workstreams were not verified in isolation on their own branches. The orchestration derailed: I gave each agent its own
CARGO_TARGET_DIRto avoid cargo lock contention, which multiplied a Skia-dependent build by five. The machine hit a load average of 75, the agents detached their compilations and then stalled waiting on them, and none delivered a report.I took the verification back myself, sequentially, on the tree where all five coexist. Three defects fell out that no agent would have seen without compiling:
cmd_rendercall site left at 9 arguments after the signature grew to 10 — the tree did not compile;trim_start()beforesplit_whitespace(), rejected byclippy -D warnings;fmtwas dirty in all five workstreams.Final state:
cargo test --workspace: 24 targets, 0 failurescargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warnings: cleanmainThe tests cover the traps named in the briefs: overdamped spring settling, undamped spring bounded, ffmpeg argument position asserted without depending on the machine running CI, and the two icon cache keys coinciding.
Found along the way, not fixed
rustmotion skills installrun inside the rustmotion repository itself duplicatesCLAUDE.md: the merge does not detect that the target is the source. Niche, but a developer testing the command in place will hit it. Worth a separate issue if it earns one.