Video panning fixes and improvements

[filip-hejsek]

This PR can be conceptually divided into two parts. The first part contains only renames and comment changes and consists of:

e7d875d87 Fix parameter names in the doc comment of VideoOutGL::Render
4ee8d3713 Clarify video sizing terminilogy
2b51b6a05 Improve some doc comments in video_display.h
741a4e536 Rename VideoDisplay::UpdateSize to FitSizeToVideo

The second part contains the actual improvements and consists of:

a89ef5cac Simplify video position calculation
3b5270116 Round position correctly when applying video panning
a9c1cd96f Implement better video zooming behavior
9003b523b Don't allow panning too far out of bounds

These changes make zooming/panning behave much nicer, especially with touchpads.

As an example, here is a comparison of mouse wheel zooming before and after. Notice how in the 'before' video, the position slips by about 5 pixels, while in the 'after' video, the position stays within 0.5px from the initial one.

Video comparison

before.mp4

after.mp4

The touchpad improvements are much more significant, but hard to show on a video without also recording the touchpad.

Also, the maximum panning distance is limited, so that you don't accidentally move the video out of view when zooming and get lost in the vast black area.

The changes weren't tested with DPI scaling. I believe I shouldn't have broken anything, but it needs to be verified.

[filip-hejsek]

The renames are of course subjective so let me know if you disagree with something.

[witchymary]

Don't allow panning too far out of bounds

This makes sense and is a good change, but I'd still like to point out that panning out ot bounds can be useful when Typesetting. Of course, you never really need to go extremely far out, but depending on how tight this limit is, it could get in the way.

[filip-hejsek]

Don't allow panning too far out of bounds

This makes sense and is a good change, but I'd still like to point out that panning out ot bounds can be useful when Typesetting. Of course, you never really need to go extremely far out, but depending on how tight this limit is, it could get in the way.

I know, that's why I used the following formula:

max_pan = 0.5 * content_size + 0.4 * viewport_size

When panning the video e.g. to the right, this ensures the distance between the left edge of the video and the right edge of the viewport is at least 10% of the viewport width.

(0.5 * content_size is the amount needed to move video edge to the center, and 0.4 * viewport_size is the additional amount needed to move it from the center to a point 10% away from viewport edge.)

I'm not particularly attached to this formula. We could lower the 10% value, or change it to be an absolute size (that might not be a bad idea actually). The formula also prevents moving the video to the corner when it is zoomed out to be smaller than 10% of the viewport, so a special case could be added for that.

Note that when you are trying to bring something far out of video bounds into view (e.g. rotation origin), it's usually better to do that by zooming out rather than panning, as when panning you don't really see where you are. And if it turns out there is a usecase that is broken by this change, we could add an option to disable the limit.

[arch1t3cht]

Thanks!

The code referred to the virtual area where the video frame is located as the 'viewport', which is not the correct use of the term.

Yeah, this mostly had historical reasons. Back when video panning wasn't possible, the viewport variables did describe the actual viewport because there was no distinction between viewport and content (though I guess even back then it was still iffy with detached video). Video panning started out as being implemented by passing different values to glViewport, so it modified those viewport variables, and the naming was never updated even after the implementation changed. Updating the terminology is definitely a good idea.

Nit 1: Typo terminilogy in the commit message
Nit 2: This commit did not rename the viewport_center_{x,y} local variables in PositionVideo. But those are removed anyway later on so it's not a huge deal - feel free to ignore this.

Rename VideoDisplay::UpdateSize to FitSizeToVideo

Carrying on with the trend of distinguishing all the various sizes we're juggling around, maybe FitClientSizeToVideo/FitWindowSizeToVideo/FitClientToVideo (whichever you prefer) might be even better?

The code also didn't take into account the fact that a touchpad or touchscreen zoom gesture should not only zoom, but also pan the video

Oh, neat! When implementing gesture support I was testing on a Mac (since none of my standard setups have both Wayland and a touchpad), and there the zoom gesture is only a zoom with no panning. So I didn't realize that wx's gestures on other platforms do also include panning. Thanks for noticing this! Touchpad zooming on Wayland does indeed feel much better now.

Comment: Can you add some comment explaining what coordinate system zoomGestureAnchorPoint is using? If I understand your (very elegant) math correctly, it's where the mouse position at the start of the gesture would map to when undoing the current zoom and pan? And the reasoning is that this point should not change throughout a gesture? If that's correct then a comment somewhere explaining something along these lines might help a lot.

Don't allow panning too far out of bounds

The limit you chose is pretty much what I would also have intuitively chosen if I had been forced to pick a limit. I don't think it should impair the user during normal typesetting: If the user needs to pan this far out of bounds then chances are that whatever they're doing is suboptimal or will have bad performance.

The one use case I can imagine where this could hurt is when trying to fix some existing broken line/clip/etc (as opposed to purposefully creating a line/clip point so far outside). Maybe a bug in Aegisub or some script created a clip or line that's positioned extremely far away, and the user want to fix it using the visual tools. But:

1. This is an edge case that requires something to already have gone wrong elsewhere.
2. The user can still fix the line manually via its text
3. This wouldn't even be a "regression" considering that up to and including the last tagged release, Aegisub did not have video panning at all.

So I think this case can safely be ignored and the panning limit can be kept.

The changes weren't tested with DPI scaling.

Thanks for mentioning this; I tested it and everything works correctly.

[filip-hejsek]

Nit 1: Typo terminilogy in the commit message

Oops, fixed.

Nit 2: This commit did not rename the viewport_center_{x,y} local variables in PositionVideo. But those are removed anyway later on so it's not a huge deal - feel free to ignore this.

This is likely a result of me first making the changes in a different order and then rewriting history to put all renames up front. Will fix.

Comment: Can you add some comment explaining what coordinate system zoomGestureAnchorPoint is using? If I understand your (very elegant) math correctly, it's where the mouse position at the start of the gesture would map to when undoing the current zoom and pan? And the reasoning is that this point should not change throughout a gesture? If that's correct then a comment somewhere explaining something along these lines might help a lot.

Yes, that's basically correct.

I have rebased the PR and will address the rest of your feedback shortly.

[filip-hejsek]

Nit 2: This commit did not rename the viewport_center_{x,y} local variables in PositionVideo. But those are removed anyway later on so it's not a huge deal - feel free to ignore this.

This is likely a result of me first making the changes in a different order and then rewriting history to put all renames up front. Will fix.

Actually, no, it was intentional. The variable actually does contain the viewport center — not the content center, as panning has not been applied to it. I admit this is a bit confusing, which is actually one of the reasons I rewrote that code.

---

I have added some comments explaining the anchor points in detail. You can review them now.

There may still be some minor inconsistencies in terminology (content vs video, screen position vs canvas position). I will try to go through the code and comments and gather all the relevant terms used, so we can consider if we want to unify them.

[filip-hejsek]

I believe I have now addressed all your feedback. I have also updated more doc comments, adding more detailed descriptions and unifying some terminology. The actual allocated area is now referred to as the client area, its size as client size, and client position is used to refer to position relative to it. Almost all uses of the term canvas have been eliminated.

[arch1t3cht]

Thanks again!