Skip to content

Fix operator precedence in the zoom-out expression - #33

Merged
shreyaskarnik merged 1 commit into
shreyaskarnik:mainfrom
geeksilva97:fix/zoom-out-operator-precedence
Jul 27, 2026
Merged

Fix operator precedence in the zoom-out expression#33
shreyaskarnik merged 1 commit into
shreyaskarnik:mainfrom
geeksilva97:fix/zoom-out-operator-precedence

Conversation

@geeksilva97

@geeksilva97 geeksilva97 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #31

I've been using Argo for a week and it's been amazing. Sending this patch to fix the abrupt zoom out.

Each phase of a camera move is built by dropping a `progress` string into
`1+${progress}*${scale-1}`. The zoom-in's progress is a single term, so it
survives that. The zoom-out's is `1-(in_time-H)/D`, which gives you

    1+1-(in_time-H)/D*(scale-1)

and ffmpeg binds `*` tighter than `+`. So for a 1.5x move the zoom-out
starts at 2.0 instead of 1.5, eases down to 1.5 instead of 1.0, then snaps
when the clause expires. The pan expressions divide by the zoom, so the
framing is off for the whole window too.

The fix is the parentheses that make `progress` one term, matching the
shape the zoom-in already has.

tests/camera-move.test.ts asserted `toContain('1+1-(in_time')` inside a
test about move chaining. The expected string looks copied from the output,
so it pinned the bug and the suite stayed green. Corrected, intent
unchanged.

The tests I added evaluate the expression instead of matching its text,
which is what would have caught this. Four of them fail without the fix.

Fixes shreyaskarnik#31
@geeksilva97
geeksilva97 force-pushed the fix/zoom-out-operator-precedence branch from 47dc6ae to d636487 Compare July 26, 2026 03:40
@geeksilva97 geeksilva97 changed the title Fix operator precedence in the zoom-out expression (#31) Fix operator precedence in the zoom-out expression Jul 26, 2026
@shreyaskarnik

Copy link
Copy Markdown
Owner

Thanks @geeksilva97 ! Really appreciate your contribution. Possible to add a before and after video of the feature so we can check the fix?

@shreyaskarnik
shreyaskarnik merged commit 4823244 into shreyaskarnik:main Jul 27, 2026
4 checks passed
@shreyaskarnik

Copy link
Copy Markdown
Owner

Merged — thanks @geeksilva97, and glad Argo's been useful 🙏

This shipped in v0.38.1, now on npm as @argo-video/cli@0.38.1.

The precedence diagnosis is exactly right, and I verified the zoom-in clause and the chained-pan lerp (easeOutExpr) were already parenthesized — so this was the only occurrence.

Particular thanks for replacing the toContain('1+1-(in_time') assertion rather than just updating it. That expectation had been copied out of the generated string rather than derived from what the curve should do, which is why it locked the bug in instead of catching it. Reading the z= expression back out and asserting on curve values — continuity at the hold boundary, arrival at 1.0, never exceeding scale — is the right level to test generated ffmpeg expressions at, and I'd like to extend that pattern to the other filter builders.

@geeksilva97

Copy link
Copy Markdown
Contributor Author

Thanks @geeksilva97 ! Really appreciate your contribution. Possible to add a before and after video of the feature so we can check the fix?

oh sorry, i missed this comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Zoom Bug

2 participants