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

EffectComposer: Introduce .setPixelRatio() and harmonize resizing. #16404

Merged
merged 1 commit into from
May 8, 2019

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 8, 2019

Successor of #14340

EffectComposer.setPixelRatio() provides more flexibilty rather than using the pixel ratio of the renderer in all cases. The parameters of EffectComposer.setSize() and WebGLRenderer.setSize() now have the same unit.

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

mrdoob commented May 8, 2019

Thanks!

@WestLangley
Copy link
Collaborator

The parameters of EffectComposer.setSize() and WebGLRenderer.setSize() now have the same unit.

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

@@ -244,7 +244,7 @@
camera.updateProjectionMatrix();

renderer.setSize( window.innerWidth, window.innerHeight );
composer.reset();
composer.setSize( window.innerWidth, window.innerHeight );
Copy link
Collaborator

Choose a reason for hiding this comment

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

As is was before.

composer.reset() should auto-size the composer based on the renderer's drawing buffer size.

@WestLangley
Copy link
Collaborator

A pixel ratio is not appropriate. EffectComposer.setSize() should take device pixels.

@WestLangley
Copy link
Collaborator

This PR should be reverted, and then I think you can get the feature you want easily.

  1. When resizing the window, typically call composer.reset(). // maybe rename it to .resize().
  2. If you want control over the composer's size when resizing the window, call composer.setSize(), passing in device pixel units.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 8, 2019

This PR should be reverted, and then I think you can get the feature you want easily.

I see this different. The user-level code is now much easier to comprehend. EffectComposer.reset() should be removed at some point. It is not used in the entire code-base anymore.

@WestLangley
Copy link
Collaborator

You said #16393 (comment) that we could discuss this issue in this PR. I was not given an opportunity to express my view.

@WestLangley
Copy link
Collaborator

I see this different. The user-level code is now much easier to comprehend.

I have pointed out that is not true. In fact, it is more complicated than it needs to be.

Pixel ratio does not make sense for the composer as it is not sized in CSS pixel units.

The composer has been designed to auto-size to the render's drawing buffer size, which is in device pixels, as it should be. It should size automatically when it is created and when the window is resized.

To accommodate users who want to size the composer differently, we have composer.setSize(), which is also in device pixels.

That is all that is needed.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 8, 2019

I think your point of view is just a different interpretation of EffectComposer's API. I think both approaches are valid but I do prefer the current one.

BTW: I find the term CSS/device pixels very confusing. Eventually, pixel ratio is nothing else than a scale factor and width and height are just pixels.

@WestLangley
Copy link
Collaborator

I find the term CSS/device pixels very confusing.

You shouldn't be using pixel ratio then.

DPR has units of (physical pixels per logical pixel) and has no bearing on EffectComposer. EffectComposer should not be sized in logical pixels, as you have done in this PR.

I think your point of view is just a different interpretation of EffectComposer's API. I think both approaches are valid

The approach prior to this PR is valid. What you have implemented here is not valid and should be reverted.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 9, 2019

The overall usage of WebGLRenderer and EffectComposer is now easier since users can now rely on a consistent approach. Configuring and resizing the render and composer is now equal and user don't have to worry about technical differences that even contributors did not understand. Otherwise I can't explain the sizing bugs in the examples prior to this PR.

TBH, I'm a bit disappointed that you don't understand the simplification this PR introduces for app-level code. And I don't buy your argumentation with physical pixels per logical pixel since it's just a different view on the same thing: Pixels and scale factors. I don't see why EffectComposer should not have a similar design like WebGLRenderer. Especially when it makes things more easier for users.

@mrdoob
Copy link
Owner

mrdoob commented May 9, 2019

I was not given an opportunity to express my view.

Ops! Sorry for not giving a window to discuss this.

I'm a bit busy this week (for the last few weeks really...) but I'll take a look as soon as the storm is over.

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