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.

Sign up for GitHub

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

Jump to bottom

New zoom algo #1415

Merged
merged 3 commits into from
Dec 17, 2020
Merged

New zoom algo #1415

merged 3 commits into from
Dec 17, 2020

Conversation

alex-w
Copy link
Member

@alex-w alex-w commented Dec 17, 2020

This patchset fixes the trackpad issue on the macbook pro. The behavior is slightly different that before so would be good to test a bit more to see if it is acceptable in all situation.

The new algo doesn't ease in the zoom animation for small changes, and ease in and out large zoom changes. The previous algorithm systematically eases in when we zoom out and systematically eases out when we zoom in. The problem with easing in small zoom changes is that the behavior then depends a lot on the frequency of wheel events, hence the trackpad bug.

Issue #529

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update

How Has This Been Tested?

Test Configuration:

  • Operating system: <Name, version number>
  • Graphics Card: <Manufacturer (likely Intel, NVidia, AMD?), Model (HD, Geforce, Radeon..., with model number), driver version?>

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@guillaumechereau
Experiment with different zoom easing curves
6c702cc 
- Replaced the custom easing curve with a QEasingCurve object, so that
  we can easily test different curves.

- Use a different scheme to select the curve, based on the duration of
  the animation.

The goal is to find a way make the zoom work properly with both mouse
wheel and macbook trackpad, hopefully without having to rely on
accumulating the events as we do now in StelApp.cpp.

Still experimental.
@guillaumechereau
Use quartic easing curve for long zoom animation
1929d93 
Seems to look a bit closer to the original behavior.
@guillaumechereau
Remove workaround for slow scrolling on trackpad
357cc0e 
Not needed anymore now that we don't smooth out the start of the easing
curve of the zoom animation.
@github-actions github-actions bot requested a review from gzotti December 17, 2020 11:18
@alex-w alex-w removed their assignment Dec 17, 2020
@github-actions
Copy link

github-actions bot commented Dec 17, 2020 •
edited by alex-w
Loading

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files

@alex-w alex-w merged commit 8743414 into master Dec 17, 2020
@alex-w alex-w deleted the new-zoom-algo branch December 17, 2020 11:20
silas1037 added a commit to silas1037/stellarium that referenced this pull request Dec 23, 2020
@silas1037
[NF] revert scale (Stellarium#1415)
0b5a83b
@silas1037
Copy link
Contributor

silas1037 commented Dec 25, 2020 •
edited
Loading

@alex-w
Have it been tested on arm device (Linux)?
#529
I scaled fail on armv7 android.
Although bugs was caused on different platforms, I afraid more bugs on cross-platforms.

@alex-w alex-w added this to the 0.20.4 milestone Dec 25, 2020
@alex-w
Copy link
Member Author

alex-w commented Dec 25, 2020

I tested on desktop linux only.

@silas1037 are you found a bottleneck for new algorithm?

@silas1037
Copy link
Contributor

silas1037 commented Dec 26, 2020 •
edited
Loading

@silas1037 are you found a bottleneck for new algorithm?

I just revert code here and got different running results.

old code: armv7 android could zoom in/out by finger
new code: zoom fail

Maybe different OS react differently to various code, but I am still interested in them to get a balance.

@gzotti
Copy link
Member

gzotti commented Dec 26, 2020 •
edited
Loading

Define "fail"? I built and ran Stellarium on my Raspberry 4 (regular current Raspberry OS, touchpad on a Logitech wireless keyboard) yesterday. Zoom with 2-finger shift works. Time drag with Ctrl+2-finger-drag works. Click (identify), double-click (center) work. What else?

I cannot comment on Android, because we don't support it.

@silas1037
Copy link
Contributor

Thanks.

@alex-w
Copy link
Member Author

alex-w commented Dec 26, 2020

@silas1037 please check the current master - probably I found source of error

@guillaumechereau
Copy link
Contributor

guillaumechereau commented Dec 27, 2020 •
edited
Loading

@alex-w: you didn't merge the new-zoom-algo branch totally it seems. Maybe that is the source of the bug?
[edit]: sorry just realized the branch was not merged but cherry picked.

@alex-w
Copy link
Member Author

alex-w commented Dec 27, 2020

you didn't merge the new-zoom-algo branch totally it seems. Maybe that is the source of the bug?

The branch was merged :) The problem is coming when duration is zero, so, I've added workaround to avoid zero in duration value.

@silas1037
Copy link
Contributor

@silas1037 please check the current master - probably I found source of error

Great. It works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gzotti gzotti Awaiting requested review from gzotti

Assignees

@guillaumechereau guillaumechereau

Labels
None yet
Milestone
0.20.4
Development

Successfully merging this pull request may close these issues.
4 participants
@alex-w @silas1037 @gzotti @guillaumechereau