Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 24, 2019

According to @WestLangley's suggestion I have added the pixel ratio to the resolution uniform of the FXAA shader.

see #15559 (comment)

@looeee
Copy link
Collaborator

looeee commented Jan 24, 2019

By the way, the SMAA example also uses the pixelRatio in composer.setSize.
Is that something that should be done for all post examples?

function onWindowResize() {
var width = window.innerWidth;
var height = window.innerHeight;
camera.aspect = width / height;
camera.updateProjectionMatrix();
renderer.setSize( width, height );
var pixelRatio = renderer.getPixelRatio();
var newWidth = Math.floor( width * pixelRatio ) || 1;
var newHeight = Math.floor( height * pixelRatio ) || 1;
composer.setSize( newWidth, newHeight );
}

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 24, 2019

I've already supposed such a change here in order to handle pixel ratio in a more consistent way: #14340

Sry, I don't want to be annoying with this PR but I'm convinced we should implement a holistic solution for this issue 😇

@looeee
Copy link
Collaborator

looeee commented Jan 24, 2019

I'm convinced we should implement a holistic solution for this issue 😇

That makes sense to me. Why did you close #14340?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 24, 2019

When a PR is not getting merged over a certain period of time (let's say several weeks) without giving any reasons, I close it. PRs are not always accepted and that's nothing special when contributing to a project. Besides, it's no big deal to reopen them^^.

@looeee
Copy link
Collaborator

looeee commented Jan 24, 2019

OK, makes sense. It might be useful to add a comment when closing along the lines of "closing due to lack of interest", for any future archaeologists.

@WestLangley
Copy link
Collaborator

According to @WestLangley's suggestion I have added the pixel ratio to the resolution uniform of the FXAA shader.

My comment was intended as a question...

This will only be correct if it is consistent with the behavior of the composer. Do you believe it is?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2019

The uniform value should match the resolution of the EffectComposers internal render targets. So if you set the pixel ratio via WebGLRenderer.setSize(), it's necessary to adjust resolution-related uniforms. Notice how getDrawingBufferSize() is used by EffectComposer's ctor.

However, if the example is resized, things break like in other post processing examples because of the reasons I have mentioned in #14340 (since EffectComposer.setSize() does not respect the pixel ratio).

I think your question shows how useful a fix of EffectComposer.setSize() is since the current behavior will confuse people over and over again.

@WestLangley
Copy link
Collaborator

How is the user supposed to know when to scale by the pixel ratio, and when the scaling will be handled for the user automatically?

Also, you should use the pixel ratio that has been set by setPixelRatio().

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2019

How is the user supposed to know when to scale by the pixel ratio, and when the scaling will be handled for the user automatically?

Ideally, users just set renderer.setPixelRatio( window.devicePixelRatio ); and the pixel ratio is also regarded by EffectComposer. The problem is that shaders might need a "resolution" uniform like the FXAA implementation the composer is not aware of. These uniforms might be named differently than "resolution". It's the responsibility of the app to set the correct values for such use cases.

Also, you should use the pixel ratio that has been set by setPixelRatio().

Done.

@WestLangley
Copy link
Collaborator

Ideally, users just set renderer.setPixelRatio( window.devicePixelRatio ); and the pixel ratio is also regarded by EffectComposer.

OK. That seems reasonable.

Of course, as we know, EffectComposer does not regard pixelRatio correctly.

@Mugen87 Mugen87 added this to the r101 milestone Jan 28, 2019
@Mugen87 Mugen87 merged commit ee696d1 into mrdoob:dev Jan 28, 2019
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