fix(validate,info): line bounding boxes, rendered duration, and four doc claims - #391
Merged
Merged
Conversation
… info matches the encoder's frame count; four stale SKILL.md claims #369: the geometry validator's viewport-overflow check reused the component's layout box for line/arrow/connector, whose painters draw straight from x1/y1/x2/y2 (or from/to) inside a canvas already translated to that box's origin. The reported bbox dropped the endpoint offset entirely, so a line's bbox was anchored at the node's own (usually 0,0) x/y with a size of just |x2-x1| by |y2-y1| — wrong on both axes, and silent on a real overflow whenever an endpoint went negative. component_bbox now derives the box from the endpoints themselves (plus half the stroke width, and the arrow-head padding already used for their intrinsic size), anchored at the layout origin the canvas is actually translated to. #372: `info` summed raw scene.duration and independently rounded each scene to frames, so it ignored legacy-v1 transition overlap and v2 `at` placement entirely, and could diverge from the video `render` actually produces. `validate` already treats build_frame_tasks(...).len() as ground truth (see validate.rs's announced_duration) — `info` now calls the same function instead of re-deriving the number, so it can no longer drift from what render/validate report. Note for whoever owns encode/video/tasks.rs: build_frame_tasks itself still rounds each scene's duration to frames independently (round(31.5) per scene rather than round(cumulative) once), so two 1.05s scenes at 30fps render 64 frames instead of 63 — this is now what `info` reports too (matching the encoder), but the encoder's own rounding is still the root cause the issue's part 2 asks to fix, and tasks.rs is outside this branch's owned files. #370: verified each of the four claims against the built binary rather than trusting the issue. All four were doc bugs, not engine bugs: - div's default border-radius renders sharp (0), not the documented 12.0 — paint_pass's decoration painter unwraps to [0.0; 4]. - `scale_x`/`scale_y` are rejected by the keyframe validator; the schema's KNOWN_MOTION_PROPERTIES only accepts `scale.x`/`scale.y` (translate_x/translate_y are fine as-is, only scale differs). - `layout.padding` (SceneLayout) is `Option<f32>` and rejects the `{top,right,bottom,left}` object form that `style.padding` (CssStyle, a different type) accepts; the doc's nearby "f32 or obj" table was for style.padding but read as if it also covered layout.padding. - `render`/`info` only accept `-f/--file`; the CLI struct in cli/mod.rs (not owned here) has no positional path argument, so the doc's `rustmotion render scenario.json ...` examples now all use `-f`. Tests: geometry::tests::line_bbox_honours_x1_y1_not_just_the_node_position, geometry::tests::line_with_negative_x1_that_pokes_off_the_left_edge_is_caught, info::duration_tests::a_v1_transition_shortens_the_rendered_total_the_way_the_encoder_sees_it, info::duration_tests::v2_at_placement_reports_the_overlapped_total_not_the_sum_of_durations.
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
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.
Closes #369. Closes #372 (partly — see below). Closes #370. Refs #388.
#369 — a line's bounding box ignored its endpoints
line,arrowandconnectorpaint at their ownx1/y1/x2/y2inside a canvas already translated to the node's layout origin, but the validator reusedbbox_of(layout): right size, wrong origin. So an overflow was reported in the wrong place, and a real one with a negativex1passed silently.The canvas-translation behaviour was confirmed empirically before trusting the issue's formula — a line at
x=300,y=200,x1=100,y1=100puts ink at(400,300), i.e.x + x1, so the endpoint offset genuinely adds to the layout origin rather than replacing it.Red proof:
Against the release binary, the issue's JSON now reports
[958, 98] -> [962, 1282]instead of[0, 0] -> [1, 1180].#372 —
infore-derived the duration instead of asking the encodervalidatealready usedbuild_frame_tasks(...).len()as ground truth.infowas the last place summingscene.duration, so it ignored v1 transition overlap and v2atplacement entirely. It now calls the same functionrenderandvalidatedo.at: 0andat: 1.0over 2.0 s scenes (v2)Part 2 of #372 is not fixed here, deliberately. A scene duration landing on a half frame adds one — two 1.05 s scenes render 64 frames, not 63 — because
build_frame_tasksrounds each scene independently and the error accumulates. That is inencode/video/tasks.rs, outside this workstream's ownership.infonow faithfully reports whatrenderwill produce, so the two no longer disagree with each other; both still disagree with what rounding the cumulative timeline once would give. I am keeping #372 open for that half rather than closing it — see the note below.#370 — four documentation claims, each verified against the binary
The brief was to establish which side was wrong for each, and to report rather than document a bug as intended behaviour. All four turned out to be the doc drifting from working behaviour — no engine bug hiding behind any of them.
divdefaults toborder-radius: 12.unwrap_or([0.0; 4])scale_x/scale_yare animatableunknown animation property 'scale_x': expected one of … scale, scale.x, scale.y;scale.xvalidateslayout.paddingtakes an objectinvalid type: map, expected f32.SceneLayout.paddingisOption<f32>;CssStyle.paddingisOption<Edges>— the doc conflated two different fieldserror: unexpected argument found;--helpshows--file <FILE>required, no positionalThe
scale_xasymmetry is real but pre-existing:translate_x/translate_yare accepted in underscore form and onlyscalediffers. Left alone — nothing is broken, the naming is just inconsistent, andschema/video.rswas out of scope.Verification
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(1594) all clean. Comment count still 0.Written comment-free, per the codebase-wide rule from #345.