OrbitControls: Support frame rate independent autoRotate.

[Issung]

Fixed #26471.

Updates the OrbitControls getAutoRotationAngle() function to take into account clock delta so that autoRotate can become framerate independent.

[Issung]

See my first commit on the branch to see some of my logic expressed in some comments, I'm pretty sure the math is equivalent.

[Issung]

Some unexpected failures here, is this typical?

[WestLangley]

You can do this in your app. See #24956 (comment).

To maintain the current default rotation rate, you would do this:

controls.autoRotateSpeed = 120 * clock.getDelta();

[Issung]

I think it would be good to make it FPS independent by default for all users of the class :)

[Mugen87]

Another discussion about this topic:

https://discourse.threejs.org/t/speed-of-orbitcontrols-autorotate-not-related-to-fps/43910

Maybe we can to try to find solution within OrbitControls to avoid further confusion. IMO, a FPS independent solution is the more appropriate default behavior.

[Mugen87]

The problem with using THREE.Clock is that when users switch to another tab and then come back to the application after a while, you end up with a huge time delta value that leads to a visible jump. Compare the PRs version with prod to see this effect:

https://raw.githack.com/Issung/three.js/%2326471-orbitcontrols-framerate-independent/examples/webgl2_volume_instancing.html
https://threejs.org/examples/webgl2_volume_instancing

This issue can only be solved with another time class implementation like #17912.

The controls now also produce large time delta values when users enable - disable - enable auto rotation.

[Mugen87]

Some unexpected failures here, is this typical?

This PR changes the behavior of autoRotate which means the screenshots are rendered differently (and thus the E2E test fails). The time delta value is 0 in the first frame which means getAutoRotationAngle() returns 0 as well.

A behavior change is not a problem in principle but a) we have to know them and b) inform the user via the migration guide if we decide to make this change.

[Issung]

Ah valid points, thanks for the githack link I didn't know that was a thing, that will be helpful fo testing. Ideally I hope to make this change in such a way that it can produce the same behaviour in legitimate situations so that we don't need a migration guide at all, just a fix patch note.

I think it may be possible to solve the issue with the Clock class but might need some additional fiddling to take into account the special cases you mentioned (tab going to sleep, user disabling/enabling autorotate). I'll investigate some solutions soon.

[Issung]

Tried another impl to fix those issues but it's still not super consistent, I would like to use that Timer class but it's been in PR for a while, any idea when it will get merged?

[Mugen87]

I would like to use that Timer class but it's been in PR for a while, any idea when it will get merged?

I think we should finally merge #17912 since I've seen apps adopting the exact same class or use something similar with a slightly different interface.

It does not make sense to leak the implementation of the Timer class into other components like OrbitControls. What I mean by that is that OrbitControls should not add visibilitychange handler but let a time class handle this aspect.

@Issung autoRotate requires the call of OrbitControls.update() in the animation loop. When using Timer, we could call its update() method in OrbitControls.update() and then just request the time delta in getAutoRotationAngle(). The change in onPointerUp() should not be necessary anymore. Does this sound right to you^^?

[Issung]

I agree totally. I just wanted to see how "ugly" my change would be in OrbitControls and the answer is too ugly for my liking, I would like to use the Timer class.
Yes how it would be implemented makes total sense to me, would be a much nicer and more stable implementation.
I have now subscribed to notifications for #17912 so as soon as it's merged I'll get to fixing this PR. Let's push it over the line! :)

[Mugen87]

@yomotsu I've checked camera-controls and realized there is no autoRotate property like in OrbitControls. What was your motivation to implement this feature on app level like shown in the respective example?

https://github.com/yomotsu/camera-controls/blob/cee042753169f3bbeb593833ce92d70d52b6862f/examples/auto-rotate.html#L112C9-L116

I wonder if we overlook a potential issue with using an alternative Clock implementation like the Timer class inside OrbitControls.

[yomotsu]

@Mugen87

Thanks for your interest.
The reason for it is there are several cases to consider, like:

• Users may want to rotate with a sine curve for the polar axis(like waving)
• Rotate with various controlled speeds (like, slow down in the front and first in the back.
• What if users drag while auto-rotating?

These depend on user requirements, and ended up I thought auto-rotate is not simple and should be handled in user-land.

[yomotsu]

By the way, can we just add a delta time argument to OrbitControl.prototype.update( delta )?
Then if the argument is undefined, just works as it is now.
So, we don't need an internal Clock or Timer.

[Mugen87]

That is a good point!

@Issung Would you be okay with that solution?

[Issung]

Passing the delta time to the update method is a nice non-breaking change that could be made, but it still has the issue of when the tab is put in the background for a few seconds, when coming back to the tab it will have a large visible jump because of the large delta time.

[Issung]

It might be a good interim solution while we wait for the Timer class PR to be merged, I could make another PR for that if you like?

[Mugen87]

Yes, it would be good to update the PR. If the time delta value is eventually computed with Timer the large delta issues should go away.

[Issung]

I was thinking of making a new seperate PR with @yomotsu 's idea, and keeping this one around for when the Timer class is reaches develop. Does that sound OK?

[Mugen87]

THB, I don't think we have to integrate timer if the update() approach works fine. Computing a time delta value once and reuse it in the app is actually favorable than computing various deltas in different modules.

[Issung]

Aright sounds good to me, I'll re-shape the PR when I get a moment 👍

[Issung]

I've simplified the PR with @yomotsu 's idea. I included the change as a demo in webgl2_volume_instancing.html, should I include it there and in other demos or just leave it out? Also is the documentation stored in the repo as well to update with the new information?

[Issung]

It seems my update to webgl2_volume_instancing is what is making the tests fail, I'll await your reply on my above question to see what action we should take

[Mugen87]

Try to regenerate the E2E screenshot via:

npm run make-screenshot webgl2_volume_instancing

Updating one example is sufficient for now. However, updating the docs would be great!

[Mugen87]

I'll merge and clean up.