Fixing 2 regressions with video_widget (Region support, Zoom support)

[jonoomph]

Fixing regressions with #4527.

• Region drawing was broken (used by Tracker effect)
• Zooming with Scroll-Wheel on video_widget was broken

I am reverting just the sections of code required to fix these 2 issues.

[JacksonRG]

Rectangle math looks right! Merge away

[JacksonRG]

If I'm reading this right, here's the error. Window is always at the top left corner.

[jonoomph]

You might be right.... but my brain struggles with this type of logic, lol

[ferdnyc]

That's not the error (I'm not sure what the error is, I didn't see what problem this was meant to address) — window_rect is the window (the widget area being displayed in), of course its origin is in its own upper-left corner, and it has the size indicated. You can't create a rect without an origin point, which is almost always QPoint(0,0) or QPointF(0.0, 0.0).

This later call centers the viewport within the window:

viewport_rect.moveCenter(window_rect.center())

[ferdnyc]

Well, anyway, like I said the lack of accounting for zoom was the only difference. Here's the entire change required to add zooming to my centeredViewport():

diff --git a/src/windows/video_widget.py b/src/windows/video_widget.py
index a4d42969..5666a9a9 100644
--- a/src/windows/video_widget.py
+++ b/src/windows/video_widget.py
@@ -513,7 +513,9 @@ class VideoWidget(QWidget, updates.UpdateInterface):
         window_rect = QRectF(QPointF(0, 0), window_size)
 
         aspectRatio = self.aspect_ratio.ToFloat() * self.pixel_ratio.ToFloat()
-        viewport_size = QSizeF(aspectRatio, 1).scaled(window_size, Qt.KeepAspectRatio)
+        viewport_size = QSizeF(aspectRatio, 1).scaled(
+                            window_size, Qt.KeepAspectRatio
+                        ) * self.zoom
         viewport_rect = QRectF(QPointF(0, 0), viewport_size)
         viewport_rect.moveCenter(window_rect.center())

After that patch, it produces the same results as this code.

(Within a pixel one way or the other, attributable to the difference between passing floats to QRect() — which I think in Python < 3.10 effectively called int() on all of the arguments, meaning it truncated floats — vs. using QRectF.toRect(), which will round to the nearest integer)

But also:

1. It doesn't break Python 3.10
2. It respects pixel aspect ratio. (Shouldn't the viewport be doing that? I feel like it should. Unless it's already applied somewhere else.)

[ferdnyc]

This change re-broke Python 3.10. you can't pass QRect() float values, it only takes ints. In Python 3.10 that's an instant crash.

If there's an issue here, it's that my code doesn't account for zoom — I thought that was intentional, I thought I'd handled that somewhere else. I'd certainly meant to, because the zoom math should be independent of the viewport. The viewport math is based on the original, un-zoomed frame image.

If it gets zoomed in or out, that just scales it larger or smaller around its own center point. Zooming changes the apparent size, not the actual size. If the viewport is going to account for zoom, then shouldn't it have to account for rotation as well? Rotation also changes the frame's apparent size (and aspect).

[ferdnyc]

(The idea here was that centeredViewport() would be generic — you give it dimensions, it tells you how to center the image in those dimensions based on its aspect ratio. Scaling, rotation, etc, are all separate operations that occur after that point.)

[ferdnyc]

Also, at the risk of ranting a bit... the code needs to stop abusing the term "heightForWidth" for simple scaling. That term has a meaning in Qt, and that's not what it means.

"HeightForWidth" means that a widget's height is dependent on its width, INDEPENDENT of scaling.

Picture a text widget containing some wrapped text. If the widget is wide enough that none of the lines wrap, N lines fit in a widget N lines tall:

But if the widget is made narrower and lines wrap, its height changes:

THAT widget would return True for .hasHeightForWidth(). Because that's what it means. Scaling a rectangle is not a height-for-width operation, it's just scaling. And it doesn't need to be done in the complicated way it's done here.

[ferdnyc]

OK, the only issue here was that I got the test backwards. It should be self.region_enabled = bool(clip_id). But it's very odd to be testing self in a Python method. self is the current class instance. It's implicit. I've always wondered what those tests were meant to accomplish.

[ferdnyc]

(The reason I got the test backwards was that it sets region_enabled to False if not clip_id is True. Just like in speech, double negatives in code are confusing.)