Check if PointsMaterial map is a WebGLRenderTarget. #8417
Conversation
Using a WebGLRenderTarget as a color map for the PointsMaterial results in console spam (THREE.WebGLRenderTarget: .repeat is now .texture.repeat.).
Only update the half client height when setSize is called.
|
If I recall correctly, It seems that the code pattern we should require now is material.map = renderTarget.texture; // not material.map = renderTarget;which, as far as I know, works. This will simplify the code a bit -- and, IMHO, makes more sense. |
|
@WestLangley You're right; when I first ran into this issue, I simply used the texture property of the render target directly which worked just fine. However, all the post processing examples for instance directly use their render targets as textures. That's why I decided to follow that style. Anyway, I support the proposed code pattern. Btw: the WebGLRenderer already contains code that treats render target as special cases: https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L2002-L2006. |
Right. And that special case can be removed. I expect the |
|
Yep, I also support @WestLangley suggested code pattern. I think the code clean up is worth the breakage. |
|
Anyone up for doing the refactoring? |
|
I'd be glad to do that. Just to be clear:
Right? I'm not sure where the breakage would happen with this refactoring, though 😕 |
Right.
It will break user's code. In which case it would be a good idea to test for |
|
Alright, I'm done. All examples now use the texture property of render targets that are used as inputs. I noticed that the following examples were already broken before these changes: Additionally, the following examples seem to be broken in r76dev.
At least for me they fail to render the hdr envmaps (I checked with an untouched build of three-dev). |
|
@WestLangley looks good? |
|
Looks good. @vanruesc Perhaps the |
|
I think it'd be possible to modify all materials that define explicit texture properties to use getters/setters instead. Checking for render targets could be done efficently this way and these changes could be reverted easily later on. I'm not sure how to check custom shader texture uniforms efficiently, though, because they are updated every frame. Got any leads on this? |
| uniforms.envMap.value = material.envMap; | ||
| uniforms.flipEnvMap.value = ( material.envMap instanceof THREE.WebGLRenderTargetCube ) ? 1 : - 1; | ||
| uniforms.flipEnvMap.value = ( material.envMap && material.envMap.renderTargetCube ) ? 1 : - 1; |
There was a problem hiding this comment.
Shouldn't this just be material.envMap instanceof THREE.CubeTexture?
I wouldn't add renderTargetCube to Texture dynamically...
There was a problem hiding this comment.
The WebGLRenderTargetCube has a Texture instance because it extends WebGLRenderTarget. CubeTexture is not involved in this check any way.
To replicate the original logic here, I had to resort to this awkward flag because there is no other way to know whether the texture at hand belongs to a WebGLRenderTargetCube.
There was a problem hiding this comment.
I see... I think we should figure out how to fix that first...
|
Hmmm, this PR ended up having too many different changes. The refactoring is great but can't be merged because other things are not ready 😕 |
|
Yea, I agree. It's kind of messed up at this point. What do you think about the changes in the examples? 85a7aef should work with the current version of three right away. I could make a fresh PR out of that. I'm a bit overwhelmed by the density of the WebGLRenderer class and I don't have enough time to dig through it to find a better solution. All I can offer right now is to open a new issue to outline the identified problem and keep hold of the details. |
That would be great! |
|
Continued in #8615. |
|
Verdammt! Noch so'n irrer Deutscher der's nicht lassen kann den Renderer aufzuräumen... :) 👍 I appreciate your efforts. |
|
@tschw Well, it's definitely worth every effort! Three is where it's at 😄 |
When using a WebGLRenderTarget as a map for the PointsMaterial, the following warning will be logged to the console every frame: THREE.WebGLRenderTarget: .repeat is now .texture.repeat.
981e0a2 fixes this by checking if the given map is a WebGLRenderTarget.
2d4b856 takes care of a small TODO note. The canvas' clientHeight property doesn't need to be divided by 2 every frame. Now its stored as a local variable and will only be updated when the setSize method is called.