search

pmndrs / drei

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

useProgress - Warning: Cannot update a component (`Loader`) while rendering a different component #314

Open nestorchura2019 opened 3 years ago

nestorchura2019 commented 3 years ago

Im loading a gltb model and a texture(lightMap),

when using the useProgress I get the error, if I don't load the texture (const texture = useTexture("balightmap2.jpg")), the error is not displayed, I think the error comes out because I'm loading two things, the glb and the texture

import React, { Suspense, useRef, useState, useEffect } from "react"
import { Canvas, useFrame } from "react-three-fiber"
import { Html, useGLTF, OrbitControls, useProgress, useTexture } from "@react-three/drei"

//3d model
function Office(props) {
  const groupHouse = useRef()
  //loafing lightMap texture
  const texture = useTexture("balightmap2.jpg")
  texture.flipY = false
  const { nodes, materials } = useGLTF("/oficina2.glb")

  return (
    <group ref={groupHouse} {...props} dispose={null}>
      <mesh material={materials.oficina} material-lightMap={texture} geometry={nodes.oficina.geometry} />
      <mesh material={materials.piso_Mat} material-lightMap={texture} geometry={nodes.piso.geometry} />
    </group>
  )
}

function Loader() {
  const { active, progress, errors, item, loaded, total } = useProgress()
  return <Html center>{progress} % loaded</Html>
}

export default function App() {
  return (
    <>
      <Canvas concurrent pixelRatio={[1, 2]} camera={{ position: [0, 3, 2.75] }}>
        <Suspense fallback={<Loader />}>
          <Office />
        </Suspense>
        <OrbitControls /* minPolarAngle={Math.PI / 2} maxPolarAngle={Math.PI / 2} */ enableZoom={true} enablePan={true} />
      </Canvas>
    </>
  )
}

Codesandbox - https://codesandbox.io/s/z8zjw?file=/src/App.js

image

joshuaellis commented 3 years ago

Hi @nestorchura2019,

Thanks for this, I believe this is an issue with some changes made in #304. @RenaudRohlinger, would you mind looking into this? Or shall I just revert that commit?

nestorchura2019 commented 3 years ago

Hi @nestorchura2019,

Thanks for this, I believe this is an issue with some changes made in #304. @RenaudRohlinger, would you mind looking into this? Or shall I just revert that commit?

thanks

RenaudRohlinger commented 3 years ago

@joshuaellis That's a known issue that has always been here. My last PR couldn't affect that part. For example with v3.3.0 : https://codesandbox.io/s/oficce-with-loading-ui-forked-1nyws?file=/src/App.js It's just an issue about some data loaded asynchronously mixed with a react setter.

I don't really know how to fix that, maybe refactor the whole hook?

joshuaellis commented 3 years ago

This isn't the case with v3.8.0 – https://codesandbox.io/s/oficce-with-loading-ui-forked-sjgs4?file=/src/App.js the example you've sent over uses a version of three that is not compatible with the version of drei.

RenaudRohlinger commented 3 years ago

@joshuaellis We did not had this error between 3.4.0 and 3.8.0 because I tried to fix it by using a separate instance of the loader for async multi loading which was wrong and broke the loader (between these 2 versions the loader was stuck at 0%). So I would not recommend reverting any commit. You can try any version before 3.4.0 and you will see the same error.

Ref introduced in 3.4.0: https://github.com/pmndrs/drei/pull/289

The only solution would be to refactor the loader. There is a setter in the DefaultManager onStart event (https://github.com/pmndrs/drei/blob/master/src/core/useProgress.tsx#L16) but if multiple elements are loaded at the same time it will throw "Cannot update a component while rendering a different component" since they are conflicting with each other because they all use the same loading manager.

As I said in my last PR the only clean fix would be to work on the loading manager of threejs. Using the defaultManager of threejs is not the best solution and my last PR is just an improvement of something unstable from the beginning that allows to handle a new instance of loading

nestorchura2019 commented 3 years ago

This isn't the case with v3.8.0 – https://codesandbox.io/s/oficce-with-loading-ui-forked-sjgs4?file=/src/App.js the example you've sent over uses a version of three that is not compatible with the version of drei.

What version of three.js is the recommended or compatible?

RenaudRohlinger commented 3 years ago

@nestorchura2019 in the meantime you could check the implementation with the official Loader component of Drei: https://codesandbox.io/s/oficce-with-loading-ui-forked-y6m2y?file=/src/App.js

It won't throw any error.

Maybe something to dig here @joshuaellis ?

joshuaellis commented 3 years ago

I'm closing this due to inactivity, it seems like a really niche issue & something weird react related, I personally have never had the issue. @nestorchura2019 if you want to look into fixing it you're more than welcome to and we can reopen the issue.

r-oland commented 3 years ago

Hi @joshuaellis! I'm running into the same problem right now and I think that I might have to disagree with you on it being a niche issue. Personally, I find myself wanting to use multiple loaders within a single component quite often. It's especially useful if you want to dispatch all the loaders within a scene in one context providing component for reusability purposes.

The solution above from @RenaudRohlinger with the drei component didn't work for me btw. It's probably a separate issue, but the completed state triggered too early with the useCubeTexture loader.

I'm relatively green when it comes to r3f so please do tell me if I'm missing something obvious here. ✌🏻

steffendolan commented 3 years ago

Im having the same problem. Any updates on this?

lukehorvat commented 1 year ago

I encountered this error recently as soon as I added a useProgress to my app. My app needs to request a lot of data, with 2-3 useLoader calls per component.

I figured out a solution that solved the issue for me though. Delay every loader.load() until the next tick of the event loop with a withNextTickLoading abstraction:

export function withNextTickLoading<T, L extends LoaderProto<T>>(Proto: L): L {
  return class NextTickLoader extends Proto {
    load(
      ...args: Parameters<Loader<T extends unknown ? any : T>['load']>
    ): void {
      setTimeout(() => super.load(...args), 0);
    }
  };
}

interface Loader<T> extends THREE.Loader {
  load(
    url: string,
    onLoad?: (result: T) => void,
    onProgress?: (event: ProgressEvent) => void,
    onError?: (event: ErrorEvent) => void
  ): unknown;
}

type LoaderProto<T> = new (...args: any) => Loader<T extends unknown ? any : T>;

Which can be used like so:

- const data = useLoader(THREE.FileLoader, url);
+ const data = useLoader(withNextTickLoading(THREE.FileLoader), url);

I applied it to @nestorchura2019's code and it seems to fix it too: https://codesandbox.io/s/oficce-with-loading-ui-fixed-zcm2fh

Just thought I'd share in case this brings us closer to identifying what the proper fix for useProgress should be.

arcasoy commented 10 months ago

I'm also running into this issue. I have a scene set up with a <GLTFSceneLoader /> component that uses useGLTF which calls a model from either an API endpoint or from a local file system. The scene is them passed to a <primitive /> object to render. Importantly, this component implements useProgress() to perform business logic after the model has loaded in.

As a sibling to the scene loader, there is a <EnvironmentContainer /> component that encapsulates the <Environment /> component, passing in a .hdr file into the files attribute.

So overall:

<Canvas>
<GLTFSceneLoader /> // contains useProgress & useGltf
<EnvironmentContainer /> // contains <Environment /> (which wraps useEnvironment)
</Canvas>

Oddly when the scene loader is calling a URL through useGLTF(), the error does not appear, but when calling a local file (i.e. './someFileName.glb'), the error described in OP's post appears. This makes me inclined to say that it's a timing/order of rendering issue, especially with @lukehorvat's solution that pushes the loader to the next tick.

Unfortunately, in my case the hooks used are built around useLoader() and I would prefer to not modify the package. I'm wondering if something like the solution could be implemented into useLoader() so logic dependent on it like useGltf() and useEnvironment() are fixed?

arcasoy commented 10 months ago

@joshuaellis We did not had this error between 3.4.0 and 3.8.0 because I tried to fix it by using a separate instance of the loader for async multi loading which was wrong and broke the loader (between these 2 versions the loader was stuck at 0%). So I would not recommend reverting any commit. You can try any version before 3.4.0 and you will see the same error.

Ref introduced in 3.4.0: #289

The only solution would be to refactor the loader. There is a setter in the DefaultManager onStart event (https://github.com/pmndrs/drei/blob/master/src/core/useProgress.tsx#L16) but if multiple elements are loaded at the same time it will throw "Cannot update a component while rendering a different component" since they are conflicting with each other because they all use the same loading manager.

As I said in my last PR the only clean fix would be to work on the loading manager of threejs. Using the defaultManager of threejs is not the best solution and my last PR is just an improvement of something unstable from the beginning that allows to handle a new instance of loading

I see this PR in the R3F repository that enables loader instances within useLoader(). Could this potentially resolve the issue, or does an instantiation of DefaultLoadingManager need to be added to resolve this? CC: @RenaudRohlinger @joshuaellis

CodyJasonBennett commented 7 months ago

That React warning is a real bug, so I'm elevating this issue.

mikeIsobarPL commented 5 months ago

I confirm, useProgress crashes whole app on start, what is teh status of this bug ?

pierroo commented 4 months ago

if that matters, useTexture is also raising the same issue in latest version 9.108.3 (in dev mode only, the red warning leaves in production but that remains scary)

lezan commented 3 months ago

I confirm the issue also with useTexture().

ghost commented 3 months ago

I also encountered the same problem when I used useGltf and useAnimations in a component and wrapped Suspense outside of it. When I tried to use useProgress to read the loading status, it would trigger this bug at a certain loading moment. But it seems to be caused by react. By the way, It is my version info: "@react-three/drei": "^9.109.5","@react-three/fiber": "^8.17.5","three": "^0.167.1","react": "^18.3.1", "react-dom": "^18.3.1",

zrooda commented 2 months ago

Also having this issue with multiple loaders in a component, rather easy to achieve with common usecases.

@lukehorvat's timeout is a clever hack but IMO going with a loader design that animates indeterminate progress avoiding useProgress() for now is preferred to somehow breaking React's render mechanism.

richi-coder commented 1 month ago

I've experienced the same, but in my case it must be because I am rendering a component (which is loading a texture) inside an imported model, so it throws the same error.