Skip to content
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

fix: Allow most basic and advanced gesture detectors together #1208

Merged
merged 5 commits into from
Dec 16, 2021

Conversation

spydon
Copy link
Member

@spydon spydon commented Dec 11, 2021

Description

Most gesture detectors work together now, except for the PanDetector or ScaleDetector together with the MultiTouchDragDetector. Maybe even the ScaleDetector will work with the MultiTouchDragDetector, will have to try.

Also simplified the example game a little bit.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors and are passing (See Contributor Guide).
  • My PR does not decrease the code coverage, or I have a very special case and explained on the PR description why this PR decreases the coverage.
  • I updated/added relevant documentation (doc comments with ///) and updated/added examples in doc/examples.
  • I have formatted my code with flutter format and the flutter analyze does not report any problems.
  • I read and followed the Flame Style Guide.
  • I have added a description of the change under [next] in CHANGELOG.md.
  • I removed the Draft status, by clicking on the Ready for review button in this PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flame users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md).
  • No, this is not a breaking change.

Related Issues

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR.

void onTapUp(int id, TapUpInfo info) {
super.onTapUp(id, info);
final touchPoint = info.eventPosition.game;
final handled = children.any((c) => c.shouldRemove);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting approach!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a TODO on the onTap* that it should return whether any child handled the tap (returned false)

}

@override
void onDoubleTap() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a very specific feature to showcase on this example

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was here already, I just moved the class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but we can always improve :P

WARNING: Both Advanced and Basic detectors detected.
Advanced detectors will override basic detectors and the later will not receive events
''',
!(game is MultiTouchDragDetector &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some tests for these new assertions?


@override
Future<void> onLoad() async {
await super.onLoad();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this, if super's onLoad is empty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was already there, I just moved the class.
But I can remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, it has @mustCallSuper on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, the question then why must it, if the super doesn't do anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember, there was some reasoning behind it though. Do you remember @erickzanardo or @luanpotter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is some FlameGame and Component mixins do initialization onLoad. so if people don't call super, those are missed, causing bugs. Thus we added the annoation so people would not make such mistake.

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@spydon spydon merged commit 5828b6f into main Dec 16, 2021
@spydon spydon deleted the spydon.allow-basic+advanced-gestures branch December 16, 2021 13:15
@flame-engine flame-engine deleted a comment Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants