-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Update scale event with custom recognizer #3782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… scale-gesture
spydon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few comments
| import 'package:flame/components.dart' hide Matrix4; | ||
| import 'package:flame/events.dart' hide PointerMoveEvent; | ||
| import 'package:flame/game.dart' hide Matrix4; | ||
| import 'package:flutter/material.dart' hide PointerMoveEvent, Matrix4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these hides really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be leftover I'll check
| bool _tryRemoving() { | ||
| // there's no more fingers | ||
| // that started dragging before _shouldBeRemoved flag was set to true. | ||
| if (_records.isEmpty && _shouldBeRemoved && isMounted) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it remove it directly even if there are fingers still left on the screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the thought behind this is that removing immediately the recognizer would abruptly end currently performed gestures which sounds like unwanted behavior to me (but let me know if that's acceptable).
I didn't check if removing immediately would do that though but I wrote a test for that so I can try and run the test if I just call removeFromParent immediately instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should send out a cancel event for the gesture when this happens.
Imagine a game where you are steering the player using some drag inputs and then you want to add a cut scene or something, so you remove the input capabilities, then if the gestures aren't ended directly the user can still move the player until they release the gesture.
| bool _tryRemoving() { | ||
| // there's no more fingers | ||
| // that started dragging before _shouldBeRemoved flag was set to true. | ||
| if (_records.isEmpty && _shouldBeRemoved && isMounted) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I don't think markForRemoval should exist, we should just call removeFromParent and then do clean-up in onRemove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
Description
Currently flame handle scale input events in a very hacky way, by getting the drag gesture recognizer data and recomputing
the data for the scale gesture. This has multiple issues :
This PR aims to fix all those issues by introducing a new gesture recognizer, which is basically just a mix of ScaleGestureRecognizer and immediateMultiDragGestureRecognizer, allowing pointers to be used for both gestures
without competing. I used the existing flutter code to write it.
I modified a bit ScaleCallbacks and DragCallbacks, so they use their original dispatcher if there is only one type
of them (it's a bit more efficient), and so they upgrade to using MultiDragScaleDispatcher if both mixins are mounted.
Transition between the two is smooth as the old dispatcher wait for all gestures it started to finish before removing itself.
Checklist
docsand added dartdoc comments with///.examplesordocs.I wonder if gesture_input.md should be updated
Breaking Change?
Related Issues
I believe it Closes #2635