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

WebGLRenderer: Add "target" argument to "getClearColor" #20818

Merged
merged 12 commits into from Dec 4, 2020

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Dec 3, 2020

Related issue: #20819

Description

When scene.background is set to a color the renderer clear color is modified because WebGLRenderer.getClearColor() returns a reference to its color instance.

Repro (can be run in console on docs page):

renderer = new THREE.WebGLRenderer();
scene = new THREE.Scene();
scene.background = new THREE.Color( 0xff0000 );

console.log( renderer.getClearColor() );
generator = new THREE.PMREMGenerator( renderer );
generator.fromScene( scene );
console.log( renderer.getClearColor() );

In dev this logs 0, 0, 0 then 1, 0, 0. With this PR both log statements print 0, 0, 0 as expected.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 4, 2020

The fix is correct since the same code style is used in various post processing passes, too.

@mrdoob What do you think about rewriting getClearColor() similar to getViewport() or getScissor().

this.getClearColor = function ( target ) {

    if ( target === undefined ) {

        console.warn( 'WebGLRenderer: .getClearColor() now requires a Color as an argument' );

        target = new Color();

    }

    return target.copy( background.getClearColor() );

};

However, this style requires some refactoring in the code base.

@mrdoob
Copy link
Owner

mrdoob commented Dec 4, 2020

@mrdoob What do you think about rewriting getClearColor() similar to getViewport() or getScissor().

Sounds good to me!

Would you like @gkjohnson to do that in this PR? Or should we leave it for a different PR?

@mrdoob mrdoob added this to the r124 milestone Dec 4, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 4, 2020

Or should we leave it for a different PR?

This one. In this way, the code base can migrated in one go.

@gkjohnson gkjohnson changed the title PMREMGenerator: Fix not resetting the clear color when scene.background is color WebGLRenderer: Add "target" argument to "getClearColor" Dec 4, 2020
@gkjohnson
Copy link
Collaborator Author

Done! Updated WebGLRenderer, docs, and examples.

I didn't touch WebGPURenderer but maybe that should be updated, too?

@mrdoob
Copy link
Owner

mrdoob commented Dec 4, 2020

Damn, this escalated quickly 😁

@gkjohnson
Copy link
Collaborator Author

Damn, this escalated quickly

Heh, it's what you wanted, right?! 😁

@mrdoob mrdoob merged commit 1ae1df9 into mrdoob:dev Dec 4, 2020
@mrdoob
Copy link
Owner

mrdoob commented Dec 4, 2020

Thanks!

@gkjohnson gkjohnson deleted the pmrem-fix branch December 4, 2020 21:49
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