fix(timing): a composited view keeps the transitions that do not overlap - #389
Merged
Merged
Conversation
Two ways a declared transition disappeared with nothing said about it. **One overlapping pair disabled every transition in the view.** This is a regression from #350: a single overlap routes the whole view through `v2_build_composited`, which walks output frames and never builds transition frames. The warning there only fired for a scene that itself overlaps, so a later scene sitting clear of everything lost its fade in silence. The composited path now emits `SlideTransition` for any pair whose only overlap is its own declared transition. The condition took a second attempt: the first version checked that both scenes were live at that frame, which never fires — the whole point of a v2 transition is that the outgoing scene is rendered *past* its own end, so it is not live there. The window is tested on its own. Measured on the issue's reproduction, three scenes where the first two overlap and the third declares a 1.0 s fade, sampling the centre pixel: before after 3.2s (0,0,255) (0,246,9) 3.5s (0,0,255) (0,115,140) 3.8s (0,0,255) (0,5,250) `(0,115,140)` at 3.5 s is exactly what the issue reports for the same two scenes without the overlapping one in front — so the fade is not merely present, it matches the uncomposited reference. The existing warning is now correctly scoped: it fires only when the overlap is *larger* than the declared transition, which is the case where the two really cannot both describe the same frames. **A transition on a view's first scene did nothing, silently.** A transition belongs to the scene being entered and the first has nothing to come from. It now warns at validate time and says where to move it — the failure mode is an author writing the fade on the wrong scene and counting frames to find out. Two tests. The first fails against the regression with the 30 missing transition frames; the second pins that a scene overlapping *beyond* its own transition still loses it, which is deliberate and stays loud. Closes #371
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 #371. Refs #388.
Case 2 is a regression I introduced in #350
A single overlapping pair routes the whole view through
v2_build_composited, which walks output frames and never builds transition frames. The warning I put there only fired for a scene that itself overlaps — so a later scene sitting clear of everything lost its fade with nothing on stderr.The composited path now emits
SlideTransitionfor any pair whose only overlap is its own declared transition.The condition took a second attempt, and the first one is worth recording. I checked that both scenes were live at that frame. That never fires: the whole point of a v2 transition is that the outgoing scene is rendered past its own end, so at frame 105 of the reproduction scene 1 (window 30–89) is not live at all. The transition window has to be tested on its own, independently of who is live.
Measured
The issue's reproduction — three scenes, the first two overlapping, the third declaring a 1.0 s fade — sampling the centre pixel:
(0, 115, 140)at 3.5 s is exactly the value the issue reports for the same two scenes without the overlapping one in front. So the fade is not merely present — it matches the uncomposited reference.The existing warning is now correctly scoped: it fires only when the overlap is larger than the declared transition, which is the case where the two genuinely cannot both describe the same frames.
Case 1: a transition on a view's first scene
A transition belongs to the scene being entered, and the first scene has nothing to come from. It was accepted and ignored. It now warns at validate time and says where to move it:
The failure mode this closes is an author writing the fade on the wrong scene and counting frames to work out why the video is 2.0 s instead of 1.6 s.
It hangs off
run_checks, notwarn_on_silent_defaults— the latter is only called byrender, which is why my first attempt printed nothing undervalidate.Verification
one_overlapping_pair_does_not_disable_the_other_scenes_transitions— fails against the regression:assertion left == right failed: scene 2 does not overlap anything and declares a 1.0s fade, so it must still get its 30 transition framesa_scene_that_overlaps_beyond_its_own_transition_still_loses_it— pins that the deliberate case stays deliberate, and stays loudcargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(1584) all clean.Written comment-free, per the codebase-wide rule from #345.