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

OrbitControls: Use scroll event delta to modulate zoom speed #27418

Merged
merged 1 commit into from Dec 22, 2023

Conversation

sciecode
Copy link
Collaborator

@sciecode sciecode commented Dec 21, 2023

This PR is an attempt to normalize zoom-speed across devices & operating systems.

I've noticed that OrbitControls zoom implementation leads to different zoom behaviors depending on the device / method of interacting with the module. This occurs because we attempt to normalize the amount that is zoomed on every tick / scroll event ( getZoomScale ).

While this was probably done in an attempt to circumvent potential differences in the way devices handle scrolling, this ends up having the exact opposite effect. This is quite easy to detect if you ever try zooming on a trackpad, as most operating systems implement a built-in damping system for it. In fact, I'm quite surprised I couldn't find any issues or complaints about this behaviour, as it has been present for many years now.

Every scroll event (or equivalent) provides a deltaY to inform the underlying page of the amount that should be scrolled, this allows the page to respond correctly to the event. By ignoring this amount, we are in practice ignoring potential changes in scroll behavior the user might've set in the operating system, as well as making built-in damping systems work incorrectly.

When the operating system has a built-in damping system, it does so by triggering extra events with increasingly smaller deltaY, but since this amount is ignored, we treat each of these events as a full scroll, which leads to extremelly fast zoom. In fact, this also affects the regular middle-mouse button scroll, if you try it, you'll notice it is extremely fast and hard to control. (Touch events are handled correctly)

I believe our current behaviour for middle mouse button scroll can also be improved, but I'll leave this discussion for a follow-up Issue/PR.

Live Example

@Mugen87 @WestLangley @gkjohnson

Signed-off-by: Guilherme Avila <3927951+sciecode@users.noreply.github.com>
@sciecode
Copy link
Collaborator Author

Also worth mentioning, I've tried to make it so the default zoom speed is equivalent to what we currently have. But, given that many cases would now respond to possible OS configs and scroll behaviour correctly, users might notice differences.

@mrdoob mrdoob added this to the r160 milestone Dec 22, 2023
@mrdoob mrdoob merged commit f163ee9 into mrdoob:dev Dec 22, 2023
11 checks passed
@mrdoob
Copy link
Owner

mrdoob commented Dec 22, 2023

This always bothered me. Thanks!

@sciecode sciecode deleted the dev-orbit-zoom branch December 22, 2023 12:59
@marcofugaro
Copy link
Contributor

After this PR, if you zoom by pinching on the trackpad, the zoom barely changed. In contrast to before, you were able to zoom on the scene much more by pinching.

Before this PR After this PR
beforee.mov
after.mov

@sciecode
Copy link
Collaborator Author

Can you inform OS/Browser? @marcofugaro

@marcofugaro
Copy link
Contributor

Sure, I am on a Macbook Air with Sonoma 14.0, on the latest Chrome 120

@sciecode
Copy link
Collaborator Author

sciecode commented Dec 22, 2023

I do know Mac OS implements built-in damping on their trackpad events, so it is expected that it is slower, for the reasons described in the PR. However, it shouldn't be that slow afterwards.

I've modified the controls_map example to try and investigate the behaviour.

controls_map

It should print some logs on the console, and inform which event is being called and the base amount of delta. I am mostly interested in the initial events (they should have higher values).

If you are able to share, perhaps I could identify any problems. If you are also able to share the logs using a mouse, that would be great.

@marcofugaro
Copy link
Contributor

marcofugaro commented Dec 22, 2023

Sure, here are the logs. The biggest delta is about 15, also note that I was pinching the most width available:
logs.txt

I don't currently have a mouse I can hook up to my macbook.

@sciecode
Copy link
Collaborator Author

Sure, here are the logs. The min/max delta is about 15, note that I was pinching the most width available: logs.txt

Eventhough the mode is correct, the values are indeed smaller than I would expect. Those values look more in line with a device with window.devicePixelRatio of 1, but I'm pretty sure your device should be 2.

I did some tests on other devices and it did appear like the values were considering the devicePixelRatio for the deltas given, but now I'm unsure. Having both mouse and trackpad logs on the same device, would help me narrow down.

But I imagine this problem is mostly related with normalizing the delta based on devicePixelRatio, my only question is if this was a mistake on my part, or if different OS/Browser combinations inform these deltas differently.

I will try to investigate this a bit further, thanks for the logs.

@marcofugaro
Copy link
Contributor

marcofugaro commented Dec 22, 2023

Eventhough the mode is correct, the values are indeed smaller than I would expect. Those values look more in line with a device with window.devicePixelRatio of 1, but I'm pretty sure your device should be 2.

Yeah I can confirm, window.devicePixelRatio on the macbook is 2.

@sciecode
Copy link
Collaborator Author

sciecode commented Dec 22, 2023

This proved to be a lot more complicated than I antecipated. There are two problems, one problem is relating to my previous comment, I still haven't found a clear answer if deltaY is supposed to account for device's pixel density, the spec says nothing about this.

But there is another problem, specifically relating to pinching on trackpads. The default behaviour, outside three is to control the page's scale, this triggers a wheel event (which is weird btw). However, the deltaY unit is clearly a lot smaller than the regular event.

This pinch deltaY unit does seem to be somewhat constant between different OS / devices. So we could apply a constant scale when we detect it, around 5-6x seems to be the appropriated amount. But this leads to yet another problem, the weird way to detect this is by checking if event.ctrlKey is set (...) Which means that yes, we can apply this factor, but we would also apply it for when someone scroll on mouse with ctrl key pressed.

I'm kinda baffled by how bad this all is to be honest, I'm starting to realize why we didn't bother using delta in the first place.

@mrdoob
Copy link
Owner

mrdoob commented Dec 22, 2023

We may need to check the user agent on this one...

@sciecode
Copy link
Collaborator Author

sciecode commented Dec 23, 2023

I took a look at Mozilla/gecko-dev, and got some answers here.

Their implementation, at least, doesn't consider pixel density for their deltaY either in pinch or regular wheel units.
They force css-pixels, so that is something we need to change already.

Now for the trackpad pinch problem, I did find a nice comment explaining how this deltaY is computed.

A crude simplification is that the deltaY computation for pinch events is completely arbitrary and has no real unit. Unlike regular wheel events, this is based on a scale factor and transformed into this arbitrary unit. But it should produce similar values to other user agent. This means there is no problem in us adapting this, by scaling it to a factor that is in line with our needs.


The only question that remains is how we go about differentiating between this pinch event and a regular wheel event with ctrl-key pressed. And I can only think of one method, but it is not great.

Since the pinch event automatically sets ctrlKey to true, and regular wheel event only set it to true if Control key was pressed, we could try to intercept keydown event for Control and if it was pressed, we treat it as regular wheel event, otherwise we assume trackpad. I did test this and it worked, however... this is finnicky.

Any stopPropagation on keydown event would break this logic, much like lil-gui does. So as soon as the user interacts with the GUI, this behavior stops working properly until the focus is returned to a non-stopPropagation DOMNode.

@sciecode
Copy link
Collaborator Author

sciecode commented Dec 23, 2023

By using eventListener capture: true we're able to intercept keydown before stopProgation, so we could make it work. But it honestly still feels a bit too hacky, and I'm sure this could break in unpredicted scenarios.

Nevertheless, here is a Live Example of all necessary changes. commit log.

If this is approved, I'll make a follow-up PR so we can patch r160.

@marcofugaro
Copy link
Contributor

The pinching behaviour looks good now! 🙌

asd.mov

@mrdoob
Copy link
Owner

mrdoob commented Dec 26, 2023

If this is approved, I'll make a follow-up PR so we can patch r160.

Lets give it a go 👍

AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
…27418)

Signed-off-by: Guilherme Avila <3927951+sciecode@users.noreply.github.com>
@emkajeej
Copy link

Since r160, zooming on a Macbook Pro is as slow as seen in @marcofugaro's comment #27418 (comment)
Both in scrollwheel behaviour as with pinch gestures.
Chrome 120, macos 14.2.1.

@mrdoob
Copy link
Owner

mrdoob commented Jan 22, 2024

Try updating to 0.160.1.

@emkajeej
Copy link

0.160.1 fixes it, thanks.

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

4 participants