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

Improve Touch (Part 2 - Multitouch) #283

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

rikner
Copy link
Contributor

@rikner rikner commented Mar 4, 2019

Motivation (current vs expected behavior)

Adds support for multitouch

Currently the branch causes huge slowdowns with multitouch, we may need to reorganise some code to make this efficient

ToDo

  • resolve performance issues when using multi touch

Please check if the PR fulfills these requirements

  • Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
  • There are no dependencies on other PRs or I have linked dependencies through Zenhub
  • The commit messages are clean and understandable
  • Tests for the changes have been added (for bug fixes / features)

janek and others added 23 commits September 5, 2018 15:09
…stamp, move MotionEvent to bottom, merge switch cases,
@rikner rikner changed the title Fix Multitouch Improve Touch (Part 2 - Multitouch) Mar 4, 2019
@rikner rikner requested review from janek and ephemer March 4, 2019 15:06
@rikner rikner changed the base branch from improve-touch-part1 to master March 5, 2019 13:41
@ephemer
Copy link
Member

ephemer commented Apr 17, 2019

@rikner what is the status of this PR? I'm marked to review it but it's missing a description etc. Can you please update it when you get a chance and mark me again for review if it's ready? I'm happy to do some work on this at some point myself too if needed

@ephemer ephemer removed their request for review April 17, 2019 16:22
@rikner
Copy link
Contributor Author

rikner commented May 2, 2019

@ephemer In the initial PR (#265) we refactored the timestamps and introduced multitouch, but had performance issues. I split the initial PR into #282 and this one. Currently the performance issues still exist when using multiple fingers. That's the most important issue we should solve.

@janek janek removed their request for review February 24, 2020 13:01
@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@5bb40a3). Click here to learn what that means.
The diff coverage is 27.35%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #283   +/-   ##
=========================================
  Coverage          ?   51.17%           
=========================================
  Files             ?       87           
  Lines             ?     3224           
  Branches          ?        0           
=========================================
  Hits              ?     1650           
  Misses            ?     1574           
  Partials          ?        0
Impacted Files Coverage Δ
Sources/UIScreen.swift 9.09% <ø> (ø)
Sources/UIView+animate.swift 78.37% <ø> (ø)
Sources/UIApplicationDelegate.swift 0% <ø> (ø)
Sources/UINavigationBarAndroid.swift 57.14% <ø> (ø)
Sources/AVPlayerItem+Mac.swift 0% <ø> (ø)
Sources/UIWindow.swift 71.79% <ø> (ø)
Sources/UIAlertAction.swift 0% <ø> (ø)
Sources/SDL2-Shims.swift 0% <ø> (ø)
Sources/DisplayLink.swift 0% <0%> (ø)
Sources/UIViewAnimationGroup.swift 63.63% <0%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bb40a3...a7719cb. Read the comment docs.

@rikner
Copy link
Contributor Author

rikner commented Feb 24, 2020

@ephemer
@janek and me merged the latest master and ran this on a device to reproduce the performance issue. As far as I remember the player got unresponsive for a while when using multiple fingers, but that's not the case anymore. We also looked at the Profiler in Android Studio and saw that the CPU usage was not different from when using single vs multi-touch. Could it be that this has been solved by some other PR?

@ephemer
Copy link
Member

ephemer commented Feb 25, 2020

Hey @rikner thanks for the update here! I would like to try it especially on a device with weak performance to see, it should be immediately obvious whether there are still issues here.

AFAIK we haven't changed anything in another PR so I'd be kind of surprised if the issues have spontaneously gone away. Also curious as to whether you tried this with our app or with the test app (which might hint whether the performance issues are affected by a larger / deeper view hierarchy). Feel free to ping me when you have time and we can have a look at this together

@rikner
Copy link
Contributor Author

rikner commented Feb 26, 2020

@ephemer @janek and me also tested it on the rather crappy samsung phone we have in the office. we did not see anything suspicious in the CPU usage. But let's compare the CPU usage between this branch and master again. I'll get to you with that.

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