pmndrs / drei

🥉 useful helpers for react-three-fiber
https://docs.pmnd.rs/drei
MIT License
8.26k stars 682 forks source link

Rendering starts too late with frameloop demand and CameraControls transitions #2005

Closed AaronClaes closed 2 months ago

AaronClaes commented 3 months ago

Problem description:

When setting the canvas frameloop to demand, the <CameraControls /> functions trigger invalidate() too late.

When the canvas is not being rendered and you do an action like this, you will notice the problem:

controls.dolly(1, true) // true for the transition to be smooth

When the transition is set to false, it is not noticeable. But with the transition, the first few frames are skipped. When de Canvas is already rendering, this problem does not happen, it is only the initial trigger.

Relevant code:

In the codesandbox below you will notice that if you give the Canvas time to stop rendering, and you then click the box, the initial transition of the camera controls is not smooth. If you keep clicking it, the following transitions (when the canvas is still running) are smooth.

https://codesandbox.io/p/sandbox/use-controls-bug-2mtn6l

Suggested solution:

I am unsure if this is a @react-three/fiber problem or something in the CameraControls component. It might even have something to do with how the camera-controls library works, but it would be nice to see this fixed, or have a workaround.

I have tried to use invalidate to start rendering earlier but did not have any success with this.

Thanks!

vmakela commented 3 months ago

The CameraControls update code looks pretty suspicious to me. It only calls controls.update for the implementation on every frame rendered, and it only forces renders on camera events. It does kind of work for animated transitions because the transitionstart event gets fired when starting the transition, but it does come with a delay.

Something like this would fix it, but I'm not sure if it is actually wanted here.

diff --git a/src/core/CameraControls.tsx b/src/core/CameraControls.tsx
index 082b967..b2dcab7 100644
--- a/src/core/CameraControls.tsx
+++ b/src/core/CameraControls.tsx
@@ -1,5 +1,6 @@
 import {
   Box3,
+  Clock,
   EventDispatcher,
   MathUtils,
   Matrix4,
@@ -16,7 +17,7 @@ import {

 import * as React from 'react'
 import { forwardRef, useMemo, useEffect } from 'react'
-import { extend, useFrame, useThree, ReactThreeFiber, EventManager } from '@react-three/fiber'
+import { extend, useThree, ReactThreeFiber, EventManager } from '@react-three/fiber'

 import CameraControlsImpl from 'camera-controls'
 import { ForwardRefComponent } from '../helpers/ts-utils'
@@ -81,18 +82,24 @@ export const CameraControls: ForwardRefComponent<CameraControlsProps, CameraCont

   const controls = useMemo(() => new CameraControlsImpl(explCamera), [explCamera])

-  useFrame((state, delta) => {
-    if (controls.enabled) controls.update(delta)
-  }, -1)
-
   useEffect(() => {
     controls.connect(explDomElement)
     return () => void controls.disconnect()
   }, [explDomElement, controls])

   useEffect(() => {
+    const clock = new Clock()
+    let animationFrame: number
+    const update = () => {
+      animationFrame = requestAnimationFrame(update)
+      const delta = clock.getDelta()
+      const needFrame = controls.update(delta)
+      if (needFrame) {
+        invalidate()
+      }
+    }
+
     const callback = (e) => {
-      invalidate()
       if (regress) performance.regress()
       if (onChange) onChange(e)
     }
@@ -111,6 +118,7 @@ export const CameraControls: ForwardRefComponent<CameraControlsProps, CameraCont
     controls.addEventListener('control', callback)
     controls.addEventListener('transitionstart', callback)
     controls.addEventListener('wake', callback)
+    animationFrame = requestAnimationFrame(update)

     return () => {
       controls.removeEventListener('update', callback)
@@ -119,6 +127,7 @@ export const CameraControls: ForwardRefComponent<CameraControlsProps, CameraCont
       controls.removeEventListener('control', callback)
       controls.removeEventListener('transitionstart', callback)
       controls.removeEventListener('wake', callback)
+      cancelAnimationFrame(animationFrame)
     }
   }, [controls, onStart, onEnd, invalidate, setEvents, regress, onChange])
AaronClaes commented 3 months ago

Is there a reason that callback is not triggered inside the onStartCb function? When I add this, the transition starts rendering way earlier. It is not 100% perfect yet but it is probably the best that can be done without having a separate animation loop like in the solution above.

const onStartCb: CameraControlsProps["onStart"] = (e) => {
    callback(e); // trigger callback here as well
    if (onStart) onStart(e);
};

EDIT: After testing this on more complex examples, it looks like on simple examples this does speed up things a bit, but on more complex examples the problem stays basically the same.

AaronClaes commented 2 months ago

Something like this would fix it, but I'm not sure if it is actually wanted here.

I am still looking into other solutions but can't find any, could your solution have a big performance impact? Or what would be an argument to not do it this way?

vmakela commented 2 months ago

I am still looking into other solutions but can't find any, could your solution have a big performance impact? Or what would be an argument to not do it this way?

It's a bit ugly, and it has an extra delay of one frame if the requestAnimationFrame callback ends up being called after rendering. A better way to implement it would probably be to somehow include it in fiber's frame loop, but I'm not sure how that should be done.

drcmda commented 2 months ago

The CameraControls update code looks pretty suspicious to me. It only calls controls.update for the implementation on every frame rendered, and it only forces renders on camera events. It does kind of work for animated transitions because the transitionstart event gets fired when starting the transition, but it does come with a delay.

Something like this would fix it, but I'm not sure if it is actually wanted here.

diff --git a/src/core/CameraControls.tsx b/src/core/CameraControls.tsx
index 082b967..b2dcab7 100644
--- a/src/core/CameraControls.tsx
+++ b/src/core/CameraControls.tsx
@@ -1,5 +1,6 @@
 import {
   Box3,
+  Clock,
   EventDispatcher,
   MathUtils,
   Matrix4,
@@ -16,7 +17,7 @@ import {

 import * as React from 'react'
 import { forwardRef, useMemo, useEffect } from 'react'
-import { extend, useFrame, useThree, ReactThreeFiber, EventManager } from '@react-three/fiber'
+import { extend, useThree, ReactThreeFiber, EventManager } from '@react-three/fiber'

 import CameraControlsImpl from 'camera-controls'
 import { ForwardRefComponent } from '../helpers/ts-utils'
@@ -81,18 +82,24 @@ export const CameraControls: ForwardRefComponent<CameraControlsProps, CameraCont

   const controls = useMemo(() => new CameraControlsImpl(explCamera), [explCamera])

-  useFrame((state, delta) => {
-    if (controls.enabled) controls.update(delta)
-  }, -1)
-
   useEffect(() => {
     controls.connect(explDomElement)
     return () => void controls.disconnect()
   }, [explDomElement, controls])

   useEffect(() => {
+    const clock = new Clock()
+    let animationFrame: number
+    const update = () => {
+      animationFrame = requestAnimationFrame(update)
+      const delta = clock.getDelta()
+      const needFrame = controls.update(delta)
+      if (needFrame) {
+        invalidate()
+      }
+    }
+
     const callback = (e) => {
-      invalidate()
       if (regress) performance.regress()
       if (onChange) onChange(e)
     }
@@ -111,6 +118,7 @@ export const CameraControls: ForwardRefComponent<CameraControlsProps, CameraCont
     controls.addEventListener('control', callback)
     controls.addEventListener('transitionstart', callback)
     controls.addEventListener('wake', callback)
+    animationFrame = requestAnimationFrame(update)

     return () => {
       controls.removeEventListener('update', callback)
@@ -119,6 +127,7 @@ export const CameraControls: ForwardRefComponent<CameraControlsProps, CameraCont
       controls.removeEventListener('control', callback)
       controls.removeEventListener('transitionstart', callback)
       controls.removeEventListener('wake', callback)
+      cancelAnimationFrame(animationFrame)
     }
   }, [controls, onStart, onEnd, invalidate, setEvents, regress, onChange])

useFrame is the ticker/frameloop. opening up new requestAnimFrame-loops can only hurt performance, you should imo never call raf and always use useFrame instead.

what is

const needFrame = controls.update(delta)

?

drcmda commented 2 months ago

quick fix

    <mesh
      onClick={() => {
        invalidate()
        requestAnimationFrame(() => controls.dolly(1, true))
      }}

imo the problem is that invalidate, by design, doesn't render. it will schedule a render. by that time .dolly, if it is sync, has already begun running.

invalidate() will pre-emptively schedule render. requestAnimationFrame(() => ... would execute the code next frame, so render and dolly are now in sync.

on demand rendering can pose challenges, since render has to be a centralized effort. but being able to control the flow with useland invalidate & raf seems acceptable.

AaronClaes commented 2 months ago

quick fix

    <mesh
      onClick={() => {
        invalidate()
        requestAnimationFrame(() => controls.dolly(1, true))
      }}

I tried it in some of the projects where I had this problem and it does work well, thanks a lot! Maybe this is something worth mentioning in this section of the docs?

It was the only issue that kept me from fully committing to on-demand rendering, so this might not be the only case where this fix is useful.

drcmda commented 2 months ago

will add a small example for keeping things in sync.

vmakela commented 2 months ago

I just realized that no frames are actually skipped, but the controls.update function just ends up being called with a huge delta if no frames have been rendered in a while. camera-controls seems to only use the delta provided in the update function for tracking time, so the time controls.dolly is called at doesn't matter. Doing all animated transitions like

invalidate()
requestAnimationFrame(() => controls.dolly(1, true))

ensures that a frame has been rendered just before starting the animation, so it keeps the delta small enough.

what is

const needFrame = controls.update(delta)

?

controls.update returns true if something has changed and new frame should be rendered. I don't think that fiber's rendering can be canceled from a useFrame callback, so it isn't useful for the current implementation.

drcmda commented 2 months ago

interesting! so cc thinks it will always render. this might be a limitation of the library. regardless, the raf trick would also take care of animations that truly start immediately.

i added the gotcha to the docs