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

[OSX] Use high-precision scrolling properties for 2D scrolling #7109

Conversation

alexanderweiss
Copy link
Contributor

  • Changed 2D scrolling to use the recommended scrollingDeltaX/Y properties. This results in fluid panning/scrolling across the map (when scrolling at low speeds). Resolves [OSX] NSScrollWheel event handling for 2D scrolling should use scrollingDeltaX/Y #6800.
  • Prevented scrolling using a traditional scroll wheel mouse from causing 2D scrolling. It will instead fall back to normal scrolling, triggering zoom (unless the "Function of scrollwheel" setting is set to "Off"). This matches behaviour of other apps like Apple Maps.

@michicc
Copy link
Member

michicc commented Jan 26, 2019

Code itself looks fine, but no real hardware here to test the functionality.

@andythenorth
Copy link
Contributor

I should probs test this eh.

@nielsmh nielsmh added this to the 1.9.0 milestone Feb 1, 2019
@andythenorth andythenorth removed their request for review February 2, 2019 08:52
Copy link
Contributor

@andythenorth andythenorth left a comment

Choose a reason for hiding this comment

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

  • scrolling changes work as expected, much smoother
  • change to scrollwheel mouse has issues:
    • it is consistent with other Apple apps, e.g. Apple maps
    • but setting reads "Function of scrollwheel: Scroll map", and that's the not the behaviour that happens (scrollwheel zooms), we can't ship that, it's confusing ;)
    • I don't know what the behaviour of scrollwheel mice is on other platforms
    • this could get horribly combinatorial quickly
    • I don't know what's expected here, I don't use a mouse, and I don't use map scroll, can't guess what fair user expectation is

The scrollwheel mouse issue is basically a tarpit, so if it makes sense, just take the first commit, drop the second?

@nielsmh
Copy link
Contributor

nielsmh commented Feb 2, 2019

On other platforms, the mouse wheel up/down is used to zoom in/out, this matches convention with other top-down strategy and simulation games. It makes sense to me that two-finger scroll on a touchpad should pan the map, while wheel scroll on a mouse should zoom.

@alexanderweiss
Copy link
Contributor Author

alexanderweiss commented Feb 2, 2019

Good points. I'd say the setting is actually a little confusing:

  • The description does refer specifically to 2D scrolling: "Enable scrolling with two-dimensional mouse-wheels". Even though it affects both 2D and non-2D scrolling (as the more generic setting name suggests).
  • The "Scroll map" option will only actually result in scrolling on macOS. On others it still zooms, as if set to "Zoom map" (as far as I can deduce from code; the values that trigger scrolling are only set on macOS).

Maybe it makes sense to change the names of the "Zoom map" and "Scroll map" options to something like "Always zoom map" and "Zoom or scroll map depending on input device" respectively. I'd then suggest setting the second option as default.

What are your thoughts on that? I'd very much prefer this behaviour as it means users don't have to change settings when they switch input devices. But maybe it's actually a separate issue. I can also remove the second commit as you suggested (and instead handle the traditional mouse wheels the same way albeit with a different multiplier)? I can then create an issue to discuss the setting further.

@PeterN
Copy link
Member

PeterN commented Feb 2, 2019

Who actually uses the scroll map feature? As far as I know, it was a hack by our Mac OS user at the time who had a touchpad. It should never have been OS specific, and it's probably a bad feature to have.

@alexanderweiss
Copy link
Contributor Author

alexanderweiss commented Feb 2, 2019

I really like it myself and use it all the time. Together with pinch-to-zoom it makes it a lot easier to move around the map using a trackpad. Not sure about other users of course.

@PeterN
Copy link
Member

PeterN commented Feb 2, 2019

Right, but aren't touchpad events different from mouse wheel events?

Touchpad should pan the map.
Scrollwheel should zoom the map.

This shouldn't need any settings, and ideally should work on all OSes where we can get this information. (I'm happy for it not to be supported in other OSes with this PR, though!)

@alexanderweiss
Copy link
Contributor Author

alexanderweiss commented Feb 2, 2019

Not really, both use NSScrollWheel events. And there's basically three classes of devices: trackpads, Magic Mouse (mouse, but with 2D touch-based scrolling) and scroll-wheel mice. For scrolling events macOS considers the Magic Mouse a trackpad. So differentiating between Magic Mouse and trackpad is hacky (it should be possible using the momentumPhase property). Scroll-wheel mice are different and should be detectable based on the event subtype (which is actually a better approach than I've used here, though I haven't tested it).

@PeterN
Copy link
Member

PeterN commented Feb 2, 2019

Right, by separate event I mean it should be detectable which it is, somehow, not necessarily a different event struct/handler system.

@alexanderweiss
Copy link
Contributor Author

alexanderweiss commented Feb 2, 2019

Yes, sorry, I was restructuring my comment to make it clearer.

The potential problem is that (as far as I can figure out based on documentation and the Internet; I'm not an expert on macOS input devices) it's not really possible to differentiate between scrolling on a trackpad or a Magic Mouse. Both are considered a trackpad for scrolling, but the latter is obviously a mouse.

So unless we resort to hacky detection methods it would result in a Magic Mouse working just like a trackpad; panning the map instead of zooming (which is the default behaviour in basically any Mac app, but may be weird for OpenTTD).

@michicc
Copy link
Member

michicc commented Feb 2, 2019

I'm in favour of defaulting 2D scrolling (on any platform where it is detectable) to map movement and normal mouse wheel scrolling to zoom, overridable to always zoom by a setting.

So basically:

Maybe it makes sense to change the names of the "Zoom map" and "Scroll map" options to something like "Always zoom map" and "Zoom or scroll map depending on input device" respectively. I'd then suggest setting the second option as default.

@andythenorth
Copy link
Contributor

+1 to michi.

The main issues seem to be:
(1) do we want to support 2D scroll (yes)
(2) the setting needs simplifying / rewording to not be confusing ;)

@nielsmh
Copy link
Contributor

nielsmh commented Feb 2, 2019

I have a mouse that has 2D scrolling, via two separate wheels: One between left and right button, and one for the thumb. And both wheels are capable of running "smooth"/stepless, i.e. high precision. If that was detected as wanting map panning by default I'd be quite annoyed, but I also really don't think any OS gives a way to discern between touch-scroll and wheel-scroll.

(Actually, I think on Windows you can register for receiving touch gestures in a way that disables the translation of those to mouse wheel events. That may be worth looking in to.)

@alexanderweiss
Copy link
Contributor Author

Yes, good point @nielsmh. Does it make sense to remove the second commit as @andythenorth suggested (so keep existing behaviour) and create a new issue to further discuss the behaviour and scrolling setting?

@nielsmh
Copy link
Contributor

nielsmh commented Feb 24, 2019

These changes only affect Mac builds so I can't really answer whether any non-touch mice would be affected by the gesture pan, as that depends on what the OS reports.
Would it be unreasonable to merge this for 1.9.0 release candidate, and if it turns out to cause problems then make a partial or full revert for release?

@michicc michicc merged commit 77ab6f8 into OpenTTD:master Feb 24, 2019
nicolas-cellier-aka-nice referenced this pull request in OpenSmalltalk/opensmalltalk-vm Dec 27, 2019
Add support for a sendWheelEvents flag that persists in the image header
and is settable via vmParameterAt: 48 put: ...32...

Support this on macOS and X11.  Any kind soul who knows Windows
is encouraged to write the analogous code for WIN32/64.
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.

None yet

5 participants