feat(zoom):digital zoom - #74
Merged
Merged
Conversation
- DigitalZoomViewport (Core): anchored zoom, pan and zoom-to-rect, clamped to the letterboxed picture, up to 8x - ZoomPanHost: wheel/pinch zoom at the cursor, drag to pan, Shift+drag or the zoom-to-area toggle draws a marquee, double-tap toggles - Bicubic filtering once zoomed past 1:1; minimap + zoom chip while zoomed - Detection boxes follow the zoom at constant stroke; old centre-only VM zoom removed
- Same ZoomPanHost as the live view, plus a zoom-to-area toggle on the video - RtspVideoView exposes FrameWidth/FrameAspect so hosts fit the picture without VM help - Minimap + level chip moved into a shared ZoomOverlay control
PR Summary by QodoAdd shared digital zoom to live and recorded video
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1. Pinching zooms around the wrong point
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+224
to
+225
| var ctrl = (e.KeyModifiers & KeyModifiers.Control) != 0; | ||
| if (WheelZoomRequiresModifier && !ctrl && !IsZoomed) return; |
There was a problem hiding this comment.
2. Zoomed videos trap page scrolling 🐞 Bug ≡ Correctness
OnPointerWheelChanged rejects an unmodified wheel only when WheelZoomRequiresModifier is enabled and the host is not already zoomed. After a user zooms in with Control held, ordinary wheel input continues changing zoom and is marked handled instead of scrolling the narrow page.
Agent Prompt
## Issue description
`WheelZoomRequiresModifier` currently permits unmodified wheel zoom whenever the viewport is already zoomed, causing the surrounding scrolling page to stop receiving wheel input.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/ZoomPanHost.cs[221-232]
## Recommended Fix
When `WheelZoomRequiresModifier` is true, return from `OnPointerWheelChanged` whenever Control is not pressed, regardless of `IsZoomed`. Keep the event unhandled on that path so the containing page can scroll.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| _lastPinchScale = e.Scale; | ||
| if (factor > 0 && Math.Abs(factor - 1.0) > 0.0001) | ||
| { | ||
| _viewport.ZoomAt(e.ScaleOrigin.X, e.ScaleOrigin.Y, factor); |
There was a problem hiding this comment.
1. Pinching zooms around the wrong point 🐞 Bug ≡ Correctness
ZoomPanHost.OnPinch passes PinchEventArgs.ScaleOrigin.X and .Y directly to DigitalZoomViewport.ZoomAt, although those values are normalized coordinates rather than host DIPs. On a typical 1000×500 video, a pinch centered at the picture center is treated as (0.5, 0.5) instead of (500, 250), so every pinch is anchored near the upper-left corner.
Agent Prompt
Issue description
`PinchEventArgs.ScaleOrigin` is normalized, but `ZoomPanHost.OnPinch` supplies it directly to viewport math that expects host-coordinate DIPs. This makes pinch zoom anchor near the top-left instead of the users' pinch midpoint.
Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/ZoomPanHost.cs[363-371]
Recommended Fix
Convert the normalized scale origin before calling `ZoomAt`, using `e.ScaleOrigin.X * Bounds.Width` and `e.ScaleOrigin.Y * Bounds.Height`. Keep the existing per-event scale ratio and handling behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| InvalidateVisual(); | ||
| } | ||
|
|
||
| private double Aspect => Host is { ContentAspect: > 0 } h ? h.ContentAspect : 16.0 / 9.0; |
There was a problem hiding this comment.
3. The map distorts non-widescreen video 🐞 Bug ≡ Correctness
ZoomMinimap.Aspect falls back to 16:9 and rendering stretches CurrentFrame to the entire minimap bounds whenever Host.ContentAspect is unavailable or stale. The live host receives this aspect from telemetry, whose view-model value is explicitly zero until dimensions arrive, even though the rendered video has already exposed its decoded frame aspect.
Agent Prompt
Issue description
The minimap uses a hard-coded 16:9 fallback when the host has no content aspect, then stretches the decoded bitmap to those bounds. A frame whose aspect is not 16:9 is therefore displayed with incorrect geometry while live telemetry is unavailable or stale.
Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/ZoomMinimap.cs[79-109]
- src/OpenIPC.Viewer.App/Controls/RtspVideoView.axaml.cs[24-36]
Recommended Fix
Use `Video.FrameAspect` as the fallback aspect when `Host.ContentAspect` is not positive, and invalidate minimap measurement when the decoded frame aspect changes. Render the frame into an aspect-preserving destination rectangle so the thumbnail and its viewport indicator use the same picture geometry.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+127
to
+132
| protected override void OnPointerPressed(PointerPressedEventArgs e) | ||
| { | ||
| base.OnPointerPressed(e); | ||
| e.Pointer.Capture(this); | ||
| MoveViewTo(e.GetPosition(this)); | ||
| e.Handled = true; |
There was a problem hiding this comment.
4. Secondary clicks move the zoomed view 🐞 Bug ≡ Correctness
ZoomMinimap.OnPointerPressed captures and recenters the viewport for every pointer press without checking whether the primary button is pressed. A right- or middle-click over the minimap consequently changes the user's zoom location, while the main zoom host correctly gates drag behavior on IsLeftButtonPressed.
Agent Prompt
Issue description
The minimap starts a move operation for every pointer press, including secondary and middle mouse buttons. Those gestures unexpectedly recenter the video and capture the pointer.
Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/ZoomMinimap.cs[127-132]
Recommended Fix
Read `e.GetCurrentPoint(this).Properties` at the start of `OnPointerPressed` and return unless `IsLeftButtonPressed` is true. Only capture the pointer and call `MoveViewTo` after that check.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Summary
#73
Related
Type
Checklist
TreatWarningsAsErrors=true).dotnet test); new Core logic has unit tests.AppreferencesCoreonly (Infrastructure / Video / Devices wired via DI in a head).Platforms tested
Screenshots / notes