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

Texture: Set needsUpdate to true in copy(). #23637

Merged
merged 2 commits into from
Mar 3, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 3, 2022

Fixed #23627.

Description

Texture.clone() requires a manual definition of Texture.needsUpdate (see #22718).

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 3, 2022

TextureSettingsTest.glb now renders like so:

image

@Mugen87 Mugen87 added this to the r139 milestone Mar 3, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2022

I think the best solution for this is to do needsUpdare = true inside Texture's copy()...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 3, 2022

I did not suggested to do that in #22718 because it does not solve the following issue:

const loader = new THREE.TextureLoader();
const texture  = loader.load( 'path/to/my/texture.png' );

const clonedTexture = texture.clone(); // needsUpdate set to true too early

However, modifying Texture.copy() will at least log a warning which makes the error more obvious:

THREE.WebGLRenderer: Texture marked for update but no image data found.

I'll update the PR.

@Mugen87 Mugen87 changed the title GLTFLoader: Add missing needsUpdate. Texture: Set needsUpdate to true in copy(). Mar 3, 2022
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 3, 2022

webgl2_multiple_rendertargets fails now...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 3, 2022

Ah the problem is that cloned textures for render targets now have a version greater 0. That will make WebGLTextures triggering a texture upload which is not valid for render targets. Fixing...

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 3, 2022

@Mugen87 if we have two textures with the same Source, what does texture.needsUpdate = true do now? In the past I've assumed that property causes a re-upload to the GPU... is that still true with a shared Source?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 3, 2022

No, this does not happen with Source anymore if setting needsUpdate to true happens in the same frame for both textures.

The are a couple of design issues in Texture that prevent the implementation from avoiding any possible overhead. E.g. to make the implementation backwards compatible, setting Texture.needsUpdate also triggers Source.needsUpdate. Besides, properties like Texture.type and Texture.format which are closely coupled to the data are still part of Texture. So depending on how you configure an instance of Texture, there might be additional GPU uploads. But not in the "standard" case.

@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2022

Thanks!

mrdoob pushed a commit that referenced this pull request Mar 3, 2022
* Texture: Set needsUpdate to true in copy().

* Fix WebGLMultipleRenderTargets.
@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2022

Cherry-picked into the master branch and included in 0.138.3.

@Mugen87 Mugen87 modified the milestones: r139, r138 Mar 4, 2022
donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* Texture: Set needsUpdate to true in copy().

* Fix WebGLMultipleRenderTargets.
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.

GLTFLoader: Regressions in PBR rendering with r138
3 participants