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

dispatch a textureload event when texture load is detected … #44

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

trusktr
Copy link
Contributor

@trusktr trusktr commented May 27, 2022

…so people know when to do things (f.e. to re-render a scene).

As an alternative to mrdoob/three.js#24145 which was rejected, this is a less ideal way to signal to users when texture load (in the perspective of ProjectedMaterial) has happened.

Also see the Discussion. Note that using a TextureLoader.load callback does not help with the issue described there.

This change allows someone to do the following and for the texture to appear on screen as expected:

// Example 1
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png')

mat.texture = tex

mat.addEventListener('textureload', () => {
  renderer.render(scene, camera) // it works.
})

Note that the following does not work:

// Example 2
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  renderer.render(scene, camera) // does *not* work, this will happen before `setInterval` inside of `addLoadListener` detects the texture load, and `this.uniforms.isTextureLoaded` is still `false` when this line runs.
})

mat.texture = tex

As an alternative to this PR, we could add a new isTextureLoaded getter/setter. If we do this instead, then the following will be a working alternative:

// Example 3
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  mat.isTextureLoaded = true // calls saveDimensions too.
  renderer.render(scene, camera) // now it works
})

mat.texture = tex

Which one do you like better? Example 1, or Example 3?

I like the method in Example 1 better because it does not present the user with an opportunity to do things wrong, whereas with the method in Example 3 the user can forget to set isTextureLoaded or they can set it at the wrong time before the texture is actually loaded.


To be honest, the cleanest solution would be if mrdoob/three.js#24145 was approved, in which case the example 2 above would work fine.

// Example 4, same as Example 2 but with updated comment
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  renderer.render(scene, camera) // now it works, because ProjectedMaterial would use texture load event instead of addLoadListener
})

mat.texture = tex

…know when to do things (f.e. to re-render a scene)
@trusktr
Copy link
Contributor Author

trusktr commented May 27, 2022

Side note, this problem is not apparent in any of the examples because they all use an infinite render loop, so because the scene is always re-rendering constantly, then eventually when the texture is loaded the re-draw shows it.

@trusktr
Copy link
Contributor Author

trusktr commented May 27, 2022

Yet another option is to remove the isTextureLoaded uniform altogether, then no other change is needed because the user can render the scene in TextureLoader.load's callback and it will work. In this case, if the user prefers not to render black color (for not-yet-loaded texture) they can avoid setting mat.texture until the texture is loaded, which is the same pattern as with all other material textures built into Three.js.

In other words, this is the already existing pattern people use in Three.js:

// Example 4
const mat = new MeshPhysicalMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  mat.map = tex // set the texture in here instead, to avoid black color.
  mat.needsUpdate = true
  renderer.render(scene, camera)
})

// mat.map = tex // Don't set it yet if we don't want to render black color.

And if we apply the same pattern to ProjectedMaterial (assuming we delete the isTextureLoaded, it looks like this:

// Example 5
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  mat.texture = tex // set the texture in here instead, to avoid black color.
  mat.needsUpdate = true
  renderer.render(scene, camera)
})

// mat.texture = tex // Don't set it yet if we don't want to render black color.

and now that aligns with existing Three.js patterns which means API consistency.

Which method do you like most? The method from Example 1, 3, or 5?

EDIT: The method in example 5 still needs to handle texture set before they've loaded, in order to call saveDimensions, so Example 5 isn't so good either then due to this unique requirement, unless we override Material.needsUpdate so that it will call saveDimensions. With this in mind, setting the texture early would be like the following (still with the isTextureLoaded uniform deleted):

// Example 6
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  mat.needsUpdate = true // signal update, so saveDimensions can be called
  renderer.render(scene, camera)
})

mat.texture = tex // set texture early, it may render black color

Unfortunately using needsUpdate means the renderer has to re-compile the material program. As an alternative, then the user would have to do this which is also less ideal:

// Example 7
const mat = new ProjectedMaterial()

const tex = new TextureLoader.load('foo.png', () => {
  mat.updateFromTexture() // make a public API, does not require re-compiling the material
  renderer.render(scene, camera)
})

mat.texture = tex // set texture early, it may render black color

Example 1 is still the simplest option here then, while mrdoob/three.js#24145 would be even better still.

@lumebot
Copy link

lumebot commented May 27, 2022

mrdoob/three.js#24145

@trusktr
Copy link
Contributor Author

trusktr commented May 27, 2022

Side note, I am making these changes in parallel in my TypeScript version, to eventually compile it to WebAssembly along with Three.js.

@trusktr
Copy link
Contributor Author

trusktr commented May 28, 2022

Oops, I forgot, this issue only happens if we set the material early before it is loaded, because that queues up the addLoadListener.

However, if the user does exactly like in Example 5, there is no issue because at the moment that texture property is set, it will see the texture is already loaded and immediately it will set uniforms.isTextureLoaded to true, and any subsequent animation frame will render correctly.

@marcofugaro marcofugaro merged commit 0129a36 into marcofugaro:master Jan 30, 2023
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

3 participants