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

feature: load events for Texture #24145

Closed
wants to merge 1 commit into from

Conversation

lumebot
Copy link
Contributor

@lumebot lumebot commented May 27, 2022

Related issue: https://discourse.threejs.org/t/38533

  • add test if approved to move forward

Description

Some APIs (f.e. libs) that receive textures employ hacks (no other choice) to detect when textures are loaded. Here is an example of code from @marcofugaro's three-projected-material:

// run the callback when the image will be loaded
export function addLoadListener(texture, callback) {
  // return if it's already loaded
  if (texture.image && texture.image.videoWidth !== 0 && texture.image.videoHeight !== 0) {
    return
  }

  const interval = setInterval(() => {
    if (texture.image && texture.image.videoWidth !== 0 && texture.image.videoHeight !== 0) {
      clearInterval(interval)
      return callback(texture)
    }
  }, 16)
}

The benefits of Textures emitting events are

  1. library authors do not have to remember to always include a needsUpdate setter in their classes and to tell end users to remember to call it after texture load
  2. end users don't have to remember set needsUpdate = true

Win win.

Real-world bug

When using three-projected-material (taking into account the above addLoadListener function), without an infinite render loop but calling requestAnimationFrame only when needed, this happens:

const mat = new ProjectedMaterial()

const texture = new TextureLoader.load('foo.jpg', () => {
  requestAnimationFrame(render) // draw when texture is loaded
})

mat.texture = texture
render() // render once initially, the texture will be missing in the render

// Note that ProjectedMaterial needs to set `this.uniforms.isTextureLoaded = true` once `addLoadListener` detects the texture to be loaded using `setInterval`

// ... later ...

// animation frame from the texture load fires, but this.uniforms.isTextureLoaded is still false so texture does not show up.

// ... later ...

// addLoadListener's setInterval loop finally detects the texture is loaded *after* the animation frame, and sets `this.uniforms.isTextureLoaded = true`, but there is no way for us to know that we need to re-render the scene.

And so what happens is we are stuck without the texture showing up (unless we use an infinite render loop, and not all apps do that if they need to save CPU and aren't constantly animating things).

The this.uniforms.isTextureLoaded line I mentioned in the comments is here.

@trusktr trusktr force-pushed the texture-load-event branch from 6e3050a to 73fdf49 Compare May 27, 2022 04:20
@Mugen87
Copy link
Collaborator

Mugen87 commented May 27, 2022

It was already outlined in the forum why this change is not ideal: https://discourse.threejs.org/t/load-events-for-texture/38533/3

I'm afraid this change goes in the wrong direction.

@Mugen87 Mugen87 closed this May 27, 2022
@lumebot
Copy link
Contributor Author

lumebot commented May 27, 2022

@Mugen87, @donmccurdy's response didn't outline why the change is not ideal. He only suggested that using async/await along with TextureLoader is a nice pattern, and I agree that it is.

However, it is impossible for a texture-receiving API (like materials, namely custom materials) to use async/await with a Texture (not a TextureLoader), leading to the downside that the only way to solve this is for lib authors to add needsUpdate APIs and for end users to remember to use those APIs, leading to more work altogether.

Furthermore, TextureLoaders instances do not fire separate events on a per texture basis (note they can handle multiple textures), making them not ideal to pass around in the first place because a receiving API will have no idea how to observe the load event for a specific texture.

I'm afraid this change goes in the wrong direction.

So far, no one has provided any actual description of why this is a bad direction, and I am the only person that has provided any reasoning as to why this is a good direction. So it would be great to have a real explanation of how TextureLoader being separated from Texture is better in practice.

I also described how this solves actual real-world problems, yet there was no response on what that code should do differently, and so far I am the only person that has detailed a simple change to Three.js that fixes the problem very simply.

So why close the issue early with the only explanation being "not a good direction" without explaining why?

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.

3 participants