Add platform-agnostic gesture recognition system (SKGestureDetector + SKGestureTracker)#326
Add platform-agnostic gesture recognition system (SKGestureDetector + SKGestureTracker)#326
Conversation
Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
mattleibow
left a comment
There was a problem hiding this comment.
@copilot please remove the dynamic surface iew from this PR and focus it on the gesture view. We should use the existing surface view pattern in the repo. Look at the other controls.
Please make sure we check and fix all bugs.
Please make sure we have fully documented this gesture view.
Please improve the code.
Keep working and iterating until it is perfect. After you have done work, go back and review. Make sure to review all the files and understand what it is trying to do and if it is doing it right.
This gesture view needs to support the pan, rotate, scale/zoom as well as tap and long press. It should have modes to tap and select then drag as well ass long press to select and drag.
This will have to work as some drawing canvas with stickers/images. The user should be able to perform operations.
Make sure to add a sample page in the demo app show all the features.
Please implement this fully and make sure we get 90% test coverage.
Also, please rebase on main.
Maybe see if we can extract some gesture engine that can be tested outside of a view and we can use it as part of future non maui platforms.
Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
Changes implemented in commits 2eccdb4 and b8e6328: Architecture Changes:
Gesture Support:
Selection Modes:
Tests: 39 unit tests (all passing) covering gesture engine Demo: Added The gesture engine is platform-agnostic and testable, enabling reuse for future non-MAUI platforms. Rebase on main is pending (cannot perform rebases in this environment). |
mattleibow
left a comment
There was a problem hiding this comment.
@copilot this is looking great. Please look at the comments
| private readonly SKGestureEngine _engine; | ||
| private SKCanvasView? _canvasView; | ||
| private IDispatcher? _dispatcher; | ||
| private IDispatcherTimer? _longPressTimer; |
There was a problem hiding this comment.
@copilot does this need to be here? Can't the engine use a system bcl timer?
There was a problem hiding this comment.
Fixed in da09be3. The IDispatcherTimer has been removed from the view. The engine now uses System.Threading.Timer internally for long press detection, making it fully self-contained and platform-agnostic.
|
|
||
| // Get canvas view and subscribe to touch | ||
| var templateChild = GetTemplateChild("PART_DrawingSurface"); | ||
| if (templateChild is SKCanvasView view) |
There was a problem hiding this comment.
We also need to add SKGLView
There was a problem hiding this comment.
Fixed in da09be3. OnApplyTemplate now supports both SKCanvasView and SKGLView:
var templateChild = GetTemplateChild("PART_DrawingSurface");
if (templateChild is SKCanvasView canvasView)
{
_canvasView = canvasView;
_canvasView.EnableTouchEvents = true;
_canvasView.Touch += OnTouch;
}
else if (templateChild is SKGLView glView)
{
_glView = glView;
_glView.EnableTouchEvents = true;
_glView.Touch += OnTouch;
}| /// <summary> | ||
| /// Represents the state of a touch point. | ||
| /// </summary> | ||
| internal readonly struct TouchState |
There was a problem hiding this comment.
@copilot these simple things can be record structs
There was a problem hiding this comment.
I kept them as regular readonly structs since record structs require IsExternalInit which isn't available in netstandard2.0. The structs are now cleaner with properties instead of fields:
internal readonly struct TouchState
{
public long Id { get; }
public SKPoint Location { get; }
public long Ticks { get; }
public bool InContact { get; }
// ...
}| private int _tapCount; | ||
| private GestureState _gestureState = GestureState.None; | ||
| private PinchState _pinchState; | ||
| private long? _selectedItemId; |
There was a problem hiding this comment.
@copilot do we need to track the selected item in the engine? This feels like something every app may want to do differently. For example, they may want to select multiple or an object or an index or an id. So we need to support this, but the sample does selection. How can we do this better?
There was a problem hiding this comment.
Fixed in da09be3. Selection tracking has been completely removed from the engine (SelectedItemId, SelectionMode, SelectionChanged all removed).
Apps now handle selection themselves in event handlers. The sample demo shows this pattern - it maintains its own _selectedSticker in the page and updates it in OnTap/OnLongPress handlers. This allows apps to implement any selection model they need (single, multiple, by object, by ID, etc.).
…y structs, remove selection tracking Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
mattleibow
left a comment
There was a problem hiding this comment.
Please help me understand
|
|
||
| private readonly Dictionary<long, TouchState> _touches = new(); | ||
| private readonly FlingTracker _flingTracker = new(); | ||
| private readonly object _syncLock = new(); |
There was a problem hiding this comment.
@copilot what is this for? Is there a way to not need this? Can we say this is only usable on the UI thread?
What was the reason for this being added?
There was a problem hiding this comment.
Fixed in ce0911e. The _syncLock was added because System.Threading.Timer fires on a thread pool thread, creating a race with UI thread access to _touches.
I've removed it by using SynchronizationContext instead:
- Engine captures
SynchronizationContext.Currenton first touch (from UI thread) - Timer callback posts to the captured context via
_syncContext.Post() - All state access now happens on the UI thread - no locks needed
The engine is now explicitly documented as UI-thread-only, which is the natural usage pattern for touch handling.
Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
- Split gestures.md (359 lines) into quick-start overview + 2 detail pages - Create gesture-events.md with detailed event reference - Create gesture-configuration.md with options and customization - Fix stale SkiaSharp.Extended.Gestures namespace references - Move Gestures entry to SkiaSharp.Extended section in TOC - Extract inline CSS from Gestures.razor into Gestures.razor.css Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ace refactor Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SKPanGestureEventArgs: Delta is now a calculated property (Location - PrevLocation); renamed PreviousLocation → PrevLocation - SKDragGestureEventArgs: renamed to Location/PrevLocation, removed StartLocation, Delta is calculated - SKScrollGestureEventArgs: DeltaX/DeltaY replaced with SKPoint Delta - SKFlingGestureEventArgs: VelocityX/Y replaced with SKPoint Velocity, DeltaX/Y with SKPoint Delta - All event args now follow consistent naming: X/PrevX pattern with calculated Delta Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iles - SKGestureDetectorTapTests.cs: tap, double-tap, long-press tests - SKGestureDetectorPanTests.cs: pan detection tests - SKGestureDetectorPinchRotationTests.cs: pinch, rotation, three-finger tests - SKGestureDetectorFlingTests.cs: fling detection and animation tests - SKGestureDetectorHoverScrollTests.cs: hover and mouse wheel tests - SKGestureTrackerDragTests.cs: drag lifecycle tests - SKGestureTrackerFlingTests.cs: fling animation tests - SKGestureTrackerZoomScrollTests.cs: double-tap zoom and scroll zoom tests - SKGestureTrackerTransformTests.cs: matrix, pivot, SetScale/SetRotation tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use full 'Previous' prefix instead of shorthand 'Prev' in SKPanGestureEventArgs and SKDragGestureEventArgs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please do a review of the code diff and update the PR description and title. Make sure to look for the staging comment and then also add it as links in the description. Also remove the regions from the tests as 1 region per test is not useful. |
|
Done — all three items addressed: 1. PR description & title updated
2. Staging links added to description 3. Removed all regions from test files (commit
Review findings (from fresh diff review):
Let me know which of these to address. |
Single-region-per-section is not useful when files are already split by area. The test file names provide sufficient organization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please fix them all. Also note, the lottie icon in the blazor sidebar is not showing. |
|
All fixed in Review findings fixed:
Blazor sidebar fix: Test fix:
204 gesture tests passing. |
…, icon - Remove dead _zoomPrevCumulative field (assigned but never read) - Gate PinchDetected/RotateDetected/FlingDetected events by feature toggles - Replace LINQ in GetActiveTouchPoints with zero-allocation loop (60Hz hot path) - Use monotonic Environment.TickCount64 instead of DateTime.Now for TimeProvider - Fix ZoomTo param names in docs (factor/focalPoint, not targetScale/pivot) - Add missing .bi-hand-index-nav-menu CSS for Gestures icon in Blazor sidebar - Fix flaky Fling_EventuallyCompletes test (was mixing real/fake time providers) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please do a thorough 3 agent review to find any issues. Make sure the API is consistent and follows .NET guidelines. Make sure the docs are up to date. |
|
Running a thorough 3-agent review now — checking API consistency against .NET guidelines, docs accuracy, and code quality. Will report back with findings. |
…null guards, dispose safety - Clamp() now handles NaN by returning min (prevents transform corruption) - Scroll zoom scaleDelta clamped to 0.01 minimum (prevents zero/negative scale from fast trackpad swipes) - TimeProvider setter throws ArgumentNullException on null (both detector and tracker) - ZoomTo throws ObjectDisposedException after Dispose (prevents timer creation after dispose) - Long press timer explicitly stopped when entering Pinching state - Fixed XML doc: canvas.Concat pattern, InvalidateSurface, singular 'property' - Added 4 new tests for null guard, dispose, and scroll zoom edge cases (208 total) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3-Agent Review Complete — All Findings FixedReview SummaryThree parallel review agents analyzed the full gesture system:
Critical Fixes (commit
|
…maui # Conflicts: # docs/docs/toc.yml # samples/SkiaSharpDemo.Blazor/Layout/NavMenu.razor # samples/SkiaSharpDemo.Blazor/Layout/NavMenu.razor.css
|
please merge in the latest from main |
|
✅ Already done — |
|
testing testing |
|
Looking into this...\n\nI examined the comment (ID: 3994455699) with body:\n\n"testing testing"\n\nClassification: question — appears to be a test comment; no action required. If you intended something else, please clarify. |
|
🟢 Monitoring active — received your test comment. |
Build fixes: - Replace Environment.TickCount64 with DateTime.UtcNow.Ticks (unavailable on netstandard2.0) - Replace ObjectDisposedException.ThrowIf with manual check (unavailable on netstandard2.0) Test fixes: - Fix xUnit2002 warnings: Assert.NotNull on SKPoint value type → Assert.NotEqual(default) - Fix CS0219 warnings: remove unused tapRaised variables New tests (9 added, 337 total): - NaN and Infinity coordinate handling - Use-after-dispose on detector (ProcessTouchDown/Move/Up) - Zero-distance touch (no pan triggered) - Reset clears all transform state - SetScale clamps to MinScale/MaxScale - Pinch and pan simultaneously both apply Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Production Readiness Review —
|
| Category | Status |
|---|---|
| .NET Naming Conventions | ✅ Excellent |
| Event Names (consistent pattern) | ✅ Consistent |
| EventArgs Naming (SK*GestureEventArgs) | ✅ Perfect |
| XML Documentation | ✅ Comprehensive |
| Property Validation | ✅ Excellent |
| netstandard2.0 Compatibility | ✅ Fixed |
| IDisposable Implementation | ✅ Correct |
One minor recommendation: Consider converting int ms properties (LongPressDuration, ZoomAnimationDuration, etc.) to TimeSpan for better .NET idiomaticity. This would be a breaking change.
Test Coverage ✅
337 tests passing (9 new edge case tests added):
- NaN/Infinity coordinate handling
- Use-after-dispose on detector
- Zero-distance touch (no false pan)
- Reset clears transform state
- SetScale clamps to MinScale/MaxScale
- Simultaneous pinch + pan
Test warnings fixed: xUnit2002 (Assert.NotNull on value type), CS0219 (unused variables).
Documentation ✅
- 3 gesture doc pages (gestures.md, gesture-events.md, gesture-configuration.md)
- No stale references to SKGestureEngine
- toc.yml correctly structured
- Blazor sample has gestures page with nav link
- All public APIs have XML docs
Remaining Low-Priority Items
- MAUI sample app (docs reference it but only Blazor demo exists)
int→TimeSpanfor timing properties (breaking change, defer to next version)
…cale ordering; convert duration properties to TimeSpan - Fix PinchDetected firing when IsPinchEnabled=false but IsPanEnabled=true - Reset _tapCount/_lastTapTicks when entering Pinching state to prevent false double-tap - Add SetScaleRange() method for atomic min/max scale configuration - Convert LongPressDuration, ZoomAnimationDuration, FlingFrameInterval, ZoomAnimationInterval from int (ms) to TimeSpan - Update all usages in implementation, tests, and MAUI sample - Add tests for all bug fixes and SetScaleRange edge cases (411 total, all passing) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When IsPanEnabled is false, the pinch focal point is now fixed to where the gesture started, preventing effective panning through zoom+move. The _pinchFocalPointOverride field is cleared when the gesture ends. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace inline _pinchFocalPointOverride with shared GetEffectiveGesturePivot helper method. Apply pivot locking to both pinch AND rotation handlers, fixing rotation drift when pan is disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds a complete, platform-agnostic gesture recognition system to
SkiaSharp.Extendedwith 15 new source files, 11 test files (181 tests), comprehensive docs, and Blazor + MAUI sample apps.Staging
Architecture
Two-layer design — both in core
SkiaSharp.Extended(netstandard2.0 + net9.0), no platform dependencies:SKGestureDetector— low-level state machine (None → Detecting → Panning/Pinching). Processes raw touch input and fires discrete gesture events. UsesTimeProviderfor deterministic testing.SKGestureTracker— high-level wrapper that manages a cumulativeSKMatrixtransform (pan/zoom/rotate), runs fling and double-tap-zoom animations, provides drag lifecycle, and feature toggles.Supported Gestures
TapDetectedDoubleTapDetectedLongPressDetectedPanDetectedPinchDetectedRotateDetectedFlingDetected/Updated/CompletedDragStarted/Updated/EndedScrollDetectedHoverDetectedKey Design Decisions
devicePixelRatio.Scale · Rotate · Translate(offset)— no view-center dependency.SetScale/SetRotationaccept an optionalpivotparameter.SKGestureDetectorOptions(detection) andSKGestureTrackerOptions(tracking), with full validation.Is*Enabledproperties.Handledproperty on tap/drag/pan EventArgs allows consumers to intercept gestures (e.g.,DragStarted.Handled = truesuppresses pan).SKGestureDetectorandSKGestureTrackerare sealed — no fragile base class issues, clean Dispose withoutDispose(bool).Test Coverage
181 tests across 11 files covering:
ArgumentOutOfRangeExceptionchecks)Documentation
docs/docs/gestures.mddocs/docs/gesture-events.mddocs/docs/gesture-configuration.mdSamples
samples/SkiaSharpDemo/Demos/Gestures/— multi-sticker canvas with pan/zoom/rotate/fling, full settings panel with all optionssamples/SkiaSharpDemo.Blazor/Pages/Gestures.razor— complete gesture demo with pointer events, configurable settings panelFiles Changed
41 files changed, +8,286 lines
source/SkiaSharp.Extended/Gestures/tests/SkiaSharp.Extended.Tests/Gestures/.github/skills/pr-monitor/)