Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jun 20, 2018

EffectComposer already uses the size of the drawing buffer and thus the pixel ratio of the renderer when creating its internal render targets and when using .reset(). The problem is that .setSize() does not work with the size of the drawing buffer and just applies the given width and height. On systems with devicePixelRatio greater 1, the post processing examples reduce their resolution when a resize is performed.

This PR ensures the current pixel ratio is respected when a EffectComposer.setSize() is used. Besides, the post processing examples now resize in a more consistent way than before.

@shianqi
Copy link

shianqi commented Aug 30, 2018

@Mugen87 Cool!

@WestLangley
Copy link
Collaborator

This is nice.

However, you are now handling these cases the same.

var pixelRatio = renderer.getPixelRatio();
var newWidth  = Math.floor( width * pixelRatio ) || 1; // multiply
var newHeight = Math.floor( height * pixelRatio ) || 1;
composer.setSize( newWidth, newHeight );
var pixelRatio = renderer.getPixelRatio();
var newWidth  = Math.floor( width / pixelRatio ) || 1; // divide
var newHeight = Math.floor( height / pixelRatio ) || 1;
composer.setSize( newWidth, newHeight );

So either they were wrong before and you fixed it, or it's wrong now.

Do we always want the pass to have the same resolution of the drawing buffer? I vaguely remember there are cases in which the answer is 'no'. Could that be the reason for the difference?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 31, 2018

I would say the mentioned code blocks are not correct. When I resize the browser on my system, the visual output looks clearly wrong.

Initial (webgl_postprocessing_ssaa):
image

After resize (notice how the lines get bigger and the texture blurry):
image

Since the variables width and height were derived from window.innerWidth and window.innerHeight, the new effective resolution of the composer was way smaller than the drawing buffer (four times on my system with a pixel ratio of two which seems a little bit extreme to me). The PR fixes this inconsistency.

Do we always want the pass to have the same resolution of the drawing buffer?

Yes, for performance reasons you might want to post-process in a lower resolution. Maybe EffectComposer becomes more flexibel by having a .setPixelRatio() method which allows to overwrite the pixel ratio of the renderer?

@WestLangley
Copy link
Collaborator

var newWidth = Math.floor( width / pixelRatio ) || 1; // divide

That just does not make sense to me, so l think what you have here is an improvement.

Maybe EffectComposer becomes more flexible by having a .setPixelRatio()

Let's not worry about that unless there is demand for it. :-)

@WestLangley
Copy link
Collaborator

So why does EffectComposer.setSize( width, height ) exist?

Should it not auto-size using

var size = this.renderer.getDrawingBufferSize();

Compare with .addPass().

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 1, 2018

So why does EffectComposer.setSize( width, height ) exist?

I think you need a way to tell EffectComposer to change its size e.g. when a resize of the window is performed. .setSize() ensures that all passes are resized as well.

@WestLangley
Copy link
Collaborator

I probably should have asked why we don't have EffectComposer.autoResize() instead. It does seem a bit odd to me that we use this pattern over and over:

renderer.setSize( width, height );
composer.setSize( width, height );

But it is OK, I guess.

Thanks, @Mugen87.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 2, 2018

Automatic resizing is actually an interesting feature. It should be considered if EffectComposer is refactored at some point.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 14, 2018

I'm actually open to implement a different approach as long as EffectComposer can handle the pixel device ratio correctly. If the current code is not satisfying, how about introducing a .setPixelRatio() method at a different place e.g. (EffectComposer)?

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.

4 participants