Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 6, 2019

I don't think it makes sense to divide width and height through the pixel ratio. Similar to the SMAA example, a multiplication is required:

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

@WestLangley
Copy link
Collaborator

Can you please explain your reasoning for this pattern?

var newWidth = Math.floor( width * pixelRatio ) || 1;
var newHeight = Math.floor( height * pixelRatio ) || 1;

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

In #16388 you used this pattern:

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

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 7, 2019

I've done this because webgl_postprocessing_ssao does not set a pixel ratio. And webgl_postprocessing_masking uses a lower resolution for effect composer right from the beginning. I just try to avoid scenarios where the initial render targets of EffectComposer respect the renderer's pixel ratio but not a resize via EffectComposer.setSize() (or vice versa). That said, I do not think EffectComposer always have to work on the same resolution like WebGLRenderer. There might be scenarios (e.g. performance issues) where you want to pick a lower resolution.

Against this backdrop, I've tested various approaches on my computer and I think introducing EffectComposer.setPixelRatio() provides most flexibility. By defining a pixel ratio for your composer, you can easily scale the resolution according to the hardware capabilities.

What do you think about this idea?

@Mugen87 Mugen87 changed the title Examples: Fix resizing in post-processing examples. Examples: Fixed pixel ratio computation in some post-processing examples. May 7, 2019
@WestLangley
Copy link
Collaborator

The short answer is I am glad you are looking at this and I agree that something needs to be done.

I've done this because webgl_postprocessing_ssao does not set a pixel ratio. And webgl_postprocessing_masking uses a lower resolution for effect composer right from the beginning.

They don't pertain to this PR. I am not sure what your point is.

That said, I do not think EffectComposer always have to work on the same resolution like WebGLRenderer. There might be scenarios (e.g. performance issues), where you want to pick a lower resolution.
I think introducing EffectComposer.setPixelRatio() provides most flexibility.

I do not like using "there might be" a use case as a reason. On the other hand, I don't feel strongly one way or the other. I certainly support you trying.

@WestLangley
Copy link
Collaborator

To me, this is the core problem: What are the units of the args in renderer.setSize( width, height )?

To me, the answer is CSS pixels.

What are the units of the args in composer.setSize( width, height )?

Device pixels? Actually, we get to decide. What is it?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 8, 2019

What is it?

Right now, it's device pixels. Ideally, EffectComposer.setSize() uses the same unit like WebGLRenderer.setSize() and thus CSS units. A consistent approach makes it easier for users to correctly render with EffectComposer. However, this will only work if EffectComposer does handle the pixel ratio. I'll try to file a PR to demonstrate a possible solution. We can continue the discussion there 👍

I do not like using "there might be" a use case as a reason

Well, my wording was a bit unfortunate. The ability to scale the resolution is definitely a proper requirement.

They don't pertain to this PR. I am not sure what your point is.

I think I made myself clear. The PR removes an inconsistency and does not conflict with others.

@Mugen87 Mugen87 merged commit 0cc10e9 into mrdoob:dev May 8, 2019
@Mugen87 Mugen87 added this to the r105 milestone May 8, 2019
@mrdoob
Copy link
Owner

mrdoob commented May 8, 2019

Thanks!

@WestLangley
Copy link
Collaborator

What are the units of the args in composer.setSize( width, height )? Device pixels?

Right now, it's device pixels. Ideally, EffectComposer.setSize() uses the same unit like WebGLRenderer.setSize() and thus CSS units.

EffectComposer has nothing to do with CSS and should not be sized in CSS units.

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.

3 participants