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

Examples: Add setSize() to more post-processing passes. #24744

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Oct 4, 2022

Related issue: -

Description

This PR adds some missing setSize() methods to post-processing passes.

BTW: There is some redundant code in the post-processing passes since it is actually not always necessary to apply the dimensions of the pass at creation time. When a pass is added to the pass chain, the pass is automatically resized by the width and height of the composer. So as long as setSize() is properly implemented, internal render targets can be defined with a default (1,1) resolution.

Comment on lines 19 to 21
this.renderTargetX = new WebGLRenderTarget( 1, 1 );
this.renderTargetX.texture.name = 'BloomPass.x';
this.renderTargetY = new WebGLRenderTarget( resolution, resolution );
this.renderTargetY = new WebGLRenderTarget( 1, 1 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will cause confusion to users without an explanation.

How about adding a comment:

this.renderTargetY = new WebGLRenderTarget( 1, 1 ); // will be resized later

Maybe there is a more-precise comment you could add.

Copy link
Collaborator Author

@Mugen87 Mugen87 Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a comment, how about we add 1 as a default parameter for width and height in WebGLRenderTragets ctor?

constructor( width, height, options = {} ) {

In this way we can just write const renderTarget = new WebGLRenderTarget(); which simplifies the code even more. We also allow other objects like geometries, materials and 3D objects to be created without arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe. I suggested that too, but removed it for simplicity. That is fine.

I still think adding the comment would be helpful, though.

Copy link
Collaborator Author

@Mugen87 Mugen87 Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then let me add the comments now and change the ctor of WebGLRenderTarget with a different PR.

@Mugen87 Mugen87 added this to the r146 milestone Oct 4, 2022
@Mugen87 Mugen87 merged commit 628ba3d into mrdoob:dev Oct 5, 2022
@joshuaellis joshuaellis mentioned this pull request Oct 31, 2022
19 tasks
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.

2 participants