WebGLRenderer: deprecated renderTarget and forceClear from render()

[maccesch]

resolves #15420

[Mugen87]

I only vote to merge the PR if the codebase (examples/docs) is also changed. Otherwise there are a lot of warnings all over the repository.

[maccesch]

Makes sense. I'm on it.

[maccesch]

Ok everything should be updated. The docs in englisch I changed as well but unfortunately I don't know Chinese...

[maccesch]

Is there anything that I need to change for this to be merged?

[mrdoob]

Thanks!

[mrdoob]

Ops! Seems like we overlooked something and this PR broke many examples.

https://raw.githack.com/mrdoob/three.js/dev/examples/#webgl_animation_cloth
https://raw.githack.com/mrdoob/three.js/dev/examples/#webgl_animation_skinning_blending
https://raw.githack.com/mrdoob/three.js/dev/examples/#webgl_camera_array
...

Do you mind taking a look?

[maccesch]

Sure!

[maccesch]

Turns out I somehow forgot to restore the render target after the shadow map was rendered. Sorry about that!

[Mugen87]

Updated the migration guide^^

https://github.com/mrdoob/three.js/wiki/Migration-Guide#r101--r102

[mrdoob]

Turns out I somehow forgot to restore the render target after the shadow map was rendered. Sorry about that!

No worries!

[Mugen87]

I have tested all examples in dev and it seems this PR broke the following demos:

---

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_effects_anaglyph.html
https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_shaders_ocean2.html

black screen and the following warning:

[.WebGL-0x7f9b460c1c00]GL ERROR :GL_INVALID_OPERATION : glDrawElements: Source and destination textures of the draw are the same.

---

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_materials_cubemap_dynamic.html

black screen and the following warning:

[.WebGL-0x7ffabc003800]RENDER WARNING: texture bound to texture unit 0 is not renderable. It maybe non-power-of-2 and have incompatible texture filtering.

---

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl2_multisampled_renderbuffers.html

black screen on the left and no warning

---

https://raw.githack.com/mrdoob/three.js/dev/examples/webgldeferred_animation.html

black screen and no warnings

---

All three examples work with the previous commit in dev: 28b6dc8

@maccesch Can you please have a look at these?

[maccesch]

Sure, on it.

[Mugen87]

Updated the list...

[maccesch]

The box selection works for me without any errors. Just to make sure: Did you rebuild first?

[Mugen87]

The build files on dev should be up to date.
Oh, you are right. The box selection example is indeed fine. #15769 was missing in the build

[Mugen87]

I think you have to ensure that render targets are at certain points unset. For example here:

three.js/examples/js/effects/AnaglyphEffect.js

Lines 145 to 148 in efe122c

	renderer.setRenderTarget( _renderTargetR );
	renderer.clear();
	renderer.render( scene, _stereo.cameraR );
	renderer.render( _scene, _camera );

The second call of WebGLRenderer.render() should not use a render target, so you have to set it to null. This would be the fix:

renderer.setRenderTarget( _renderTargetR );
renderer.clear();
renderer.render( scene, _stereo.cameraR );

renderer.setRenderTarget( null ); // FIX
renderer.render( _scene, _camera );

I guess the other demos needs similar fixes.

[Mugen87]

@mrdoob @WestLangley I guess we were not aware of this side effect. Problems like mentioned in my previous post will break app code.

[WestLangley]

I was aware of it. See #15420 (comment).

[WestLangley]

Who is responsible for updating the typescript files when the API is changed?

[Mugen87]

Who is responsible for updating the typescript files when the API is changed?

I would say the one who is doing the change. But for this one, see #15783

In any event, let me update the migration guide to make things more clear.

[Mugen87]

This one is also broken: https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_furnace_test.html

The spheres should not be visible.

[WestLangley]

@mrdoob What is the pattern you want users to use? In other words, what is the convention we are adopting?

renderer.setRenderTarget( null ); // always set prior to each render call
renderer.render( scene, camera );

or

renderer.getRenderTarget( previousRenderTarget );
renderer.setRenderTarget( newRenderTarget );
renderer.render( scene, camera );
renderer.setRenderTarget( previousRenderTarget ); // always restore to previous render target

[Mugen87]

In the sense that when a user uses the library he would write code like that. Also when someone copies an example this is the code he would change. Is that not correct?

That's correct, yes.

About the other point: It's more about implementing a policy like previously mentioned here: #15571 (comment). I would save/restore the render target at the place where it is actually changed. Along other properties like clear alpha or clear color.

[jee7]

Sorry to bug in, but this seems like a weird change. Having the renderTarget in the render signature was very convenient. From the user's perspective I would not want to write 4 lines of code

renderer.setRenderTarget(renderTarget);
renderer.render(scene, camera);
renderer.setRenderTarget(null);
renderer.render(scene, camera);

instead of just writing 2 lines:

renderer.render(scene, camera, renderTarget);
renderer.render(scene, camera);

Especially if I do not want to think about the state-machine with the _currentRenderTarget inside the library code. I understand that there are cases where it might make more sense to think about the state-machine, like if you need to do several ops (render, clear etc) with a single render target in a sequence.
But for me this change just seems to generate a lot of overhead in coding. Would it be possible to keep the render(scene, camera, renderTarget, forceClear) signature together with this change to support both use cases?
Or even just render(scene, camera, renderTarget)? It makes sense to read that we render a scene (what) with a camera (with what) to the render target (to where). This single line seems to indicate to the reader the main things they need to know about this render. With the other approach they always have to check if the state-machine has changed (is there some setRenderTarget maybe some 30 lines above the actual render call etc).

I see the issue this entire thing started from. That this way it was confusing to clear a render target without calling render(). However, that could easily be solved by implementing the clearRenderTarget(renderTarget) in a way that it changes the inner _currentRenderTarget back to whatever it was before the method was called. Just as it seems to be done now inside the CubeCamera.clear().

Having a state-machine in the library does not have to mean that the users must also use the library as a state-machine. Just like WebGL is a state-machine, but Three.js abstracts it away for the user.

[mrdoob]

Sorry for the delay and thanks for the patience and discourse guys!

In my eyes, the actual issue here is renderTarget.activeCubeFace. I think I would, instead, pass the activeCubeFace as a second parameter in setRenderTarget().

The CubeCamera code would look like this:

this.update = function ( renderer, scene ) {

    if ( this.parent === null ) this.updateMatrixWorld();

    var currentRenderTarget = renderer.getRenderTarget();

    var renderTarget = this.renderTarget;
    var generateMipmaps = renderTarget.texture.generateMipmaps;

    renderTarget.texture.generateMipmaps = false;

    renderer.setRenderTarget( renderTarget, 0 );
    renderer.render( scene, cameraPX );

    renderer.setRenderTarget( renderTarget, 1 );
    renderer.render( scene, cameraNX );

    renderer.setRenderTarget( renderTarget, 2 );
    renderer.render( scene, cameraPY );

    renderer.setRenderTarget( renderTarget, 3 );
    renderer.render( scene, cameraNY );

    renderer.setRenderTarget( renderTarget, 4 );
    renderer.render( scene, cameraPZ );

    renderTarget.texture.generateMipmaps = generateMipmaps;

    renderer.setRenderTarget( renderTarget, 5 );
    renderer.render( scene, cameraNZ );

    renderer.setRenderTarget( currentRenderTarget );

};

[mrdoob]

@Mugen87

When users copy code from the examples (and that happens a lot), setRenderTarget( null ); is not robust.

That's a fair point. Should we introduce renderer.unsetRenderTarget()?

[mrdoob]

@jee7

Sorry to bug in, but this seems like a weird change. Having the renderTarget in the render signature was very convenient. From the user's perspective I would not want to write 4 lines of code

It was indeed convenient, but it also confused users.

As per brevity of code, I'm hoping we'll eventually create something like what I proposed here.

So your 2 lines of code will be even shorter:

renderTarget.render( scene, camera );
renderer.render( scene, camera );

[jee7]

As per brevity of code, I'm hoping we'll eventually create something like what I proposed here.

So your 2 lines of code will be even shorter:

renderTarget.render( scene, camera );
renderer.render( scene, camera );

But that is even more confusing. In that case both the WebGLRenderTarget and WebGLRenderer would have the functionality to render. So far it has been very clear that the renderers render. If other objects with different semantics also have the ability to start the GPU pipeline, then this does not seem right. Unless you plan to fully get rid of the idea that a renderer renders. In that case there should also be some screen class like:

var screen = new THREE.WebGLScreen(...);
screen.render(scene, camera);

var target = new THREE.WebGLRenderTarget(...);
target.render(scene, camera);

Even in that case it seems weird. Does a target or a screen really should have functionality to render? They both seem rather like containers for the end result, rather than the objects that actually know how to call the pipeline and render.

The final example in your post does keep the idea that renderers render, but it still mixes apples and oranges. Like then there should also be a ScreenRenderer. Unless that is the RenderTargetRenderer with a default renderTarget = null in it.

In both cases I don't currently see how does the current WebGLRenderer fit into all that new structure. Likely it would not even be a renderer, but some context object for the new target-specific renderers. Which is an interesting concept, because then you could also have something like WebGLContext, CanvasContext etc and those all can be used to render into a render target or screen easily. But this seems like a really big change.

I hope it all works out somehow, but currently it seems difficult to see the logic behind this.

[timothyallan]

I also find it a bit off semantically if my renderTarget would have to render itself. It's a target for a renderer, not a renderer itself.

I would assume that the renderer would do the rendering, and I would tell it -what- to render, explicitly. Relying on some other function that had been called previously, maybe even by a different component/class, could be a source of confusion.

Has there been any thoughts into using a parameter object instead of hardcoded parameters like there is with Mesh's etc? No set parameter order, allows for future expansion, is explicit, and doesn't rely on multiple function calls in specific order.

Render thyself

renderer.render( scene, camera );

Render something else using optional property object

renderer.render( scene, camera, { target : renderTarget, clear : false });

or if you want to get fancy and replace

renderTarget.render( scene, camera );
renderer.render( scene, camera );

into a one liner, it could take a target array and render them in order ;)

renderer.render( scene, camera, { target : [renderTarget, renderer]});

[mrdoob]

Okay, we'll cross that bridge when we come to it.
For now we need to move the renderTarget parameter out of render() as per #15420.

[Mugen87]

After debugging for a while now I think I know why webgldeferred_animation is broken. To make it short: Calling renderer.clear() is not always the same than passing forceClear to WebGLRenderer.render(). Why? Because executing renderer.clear() without parameters clears everything (color, depth, stencil). However, this might be unwanted!

In a scenario like this, things are going to break:

renderer.autoClearDepth = false;
renderer.autoClearStencil = false;

renderer.clear(); // clears everything

renderer.render( scene, camera, renderTarget, forceClear ); // only clears color

In context of webgldeferred_animation, the problem is that RenderPass always calls renderer.clear() although WebGLDeferredRenderer changes autoClearDepth and autoClearStencil at various places. Changing the code in RenderPass to the following fixes the issue:

if ( this.clear ) renderer.clear( renderer.autoClearColor, renderer.autoClearDepth, renderer.autoClearStencil );

But this solutions seems a hack from my point of view. Maybe WebGLRenderer.clear() can respect the auto properties directly?

[Mugen87]

To sum it up: Just calling renderer.clear() is no 100% surrogate for the forceClear parameter.

[Mugen87]

Should we introduce renderer.unsetRenderTarget()?

I actually wanted to vote for the following style. It's just a more safe approach when users copy code.

var previousRenderTarget = renderer.getRenderTarget();
renderer.setRenderTarget( newRenderTarget );
renderer.render( scene, camera );
renderer.setRenderTarget( previousRenderTarget );

[mrdoob]

Thanks for investigating this @Mugen87. It's great to be aware of that breakage, but it gets to show how fragile the current design was. I think the new approach will make the code more clear.

[Mugen87]

In this case we should remove WebGLDeferredRenderer since it is based on are workflow which is not supported anymore. It seems not possible to fix WebGLDeferredRenderer without changing other entities like RenderPass. The class only works if the post-processing respects autoClearDepth and autoClearStencil . And that's not true anymore since passes clear all buffers.

An other option might be to change the way of how WebGLRenderer.clear() works. The library could allow to define a "clear state" that is respected by all calls of WebGLRenderer.clear(). Right now, you always have to pass parameters to the method call. Instead we could do this:

renderer.setClearState( true, false, false ); // only clear color but not the depth and stencil buffer
renderer.clear();

This would be closer to the "state machine" concept. The former parameters of WebGLRenderer.clear() should deprecated with such an approach.

[maccesch]

@Mugen87 Thanks for that insight on WebGLDeferredRenderer! I didn't quite get that before.

Do you think that instead of having just a broad .clear flag on RenderPass adding more fine grained flags .clearStencil, clearDepth, clearColor would be a bad idea?

[Mugen87]

I think a quick fix like this would be okay:

if ( this.clear ) renderer.clear( renderer.autoClearColor, renderer.autoClearDepth, renderer.autoClearStencil );

This approach is also used at other places like:

three.js/src/renderers/webgl/WebGLBackground.js

Line 47 in b863277

renderer.clear( renderer.autoClearColor, renderer.autoClearDepth, renderer.autoClearStencil );

The code could be added in RenderPass and ShaderPass but it would not solve the actual issue.

Do you think that instead of having just a broad .clear flag on RenderPass adding more fine grained flags .clearStencil, clearDepth, clearColor would be a bad idea?

clearColor does already exist with a different semantic. It's not a boolean but a color object.

[gfodor]

It looks like this PR introduced references to arguments inside of render, which causes memory to be allocated every frame.

[Mugen87]

Can you please elaborate in more detail what you mean?

[gfodor]

sure. the use of arguments in a JS function allocates memory (if you use the firefox allocations profiler, you can see this.) by introducing the use of arguments in render in this PR, a call to render() will now allocate memory whereas before it did not. I don't know in general three.js's philosophy on managing memory allocations but it seems a good idea to avoid it in anything happening every frame since it increases GC. there are other places where three.js allocates memory in the main render loop, but this is a recent regression and surfaces in the top allocators in profiling our app, so I wanted to mention it.

[maccesch]

@Mugen87 Should we then maybe add the arguments back but as renderTargetDeprecated and clearDeprecated or so? This way we could avoid using arguments.

[Mugen87]

@gfodor I'm afraid we have to process the deprecated parameters in some way, otherwise a lot of code will break. User should still be able to pass renderTarget and clear to the method but they are going to be warned that this approach is deprecated.

Um, this issue is an unfortunate side effect nobody was aware of.

Should we then maybe add the arguments back but as renderTargetDeprecated and clearDeprecated or so?

This is the only solution I can think of right now, too. It will clutter the method signature but at least the memory allocation is gone. And we don't have to document these parameters.

@mrdoob What do you think?

[takahirox]

Does anyone here know if the modern JIT compilers still allocate memory every frame for arguments[2,3] access? I tried to take memory allocation profile but I couldn't do it well because of the devtool change...

If they still allocate, I would like to suggest to avoid it. For example we might simply remove the checking arguments[2, 3] lines because two years has past since this PR was merged. Or *Deprecated arguments solution still looks good to me.

[mrdoob]

@takahirox removing these sounds good to me 👍