UnrealBloomPass: Unify the display effect of pc and mob. #20798
Conversation
| @@ -190,7 +190,7 @@ UnrealBloomPass.prototype = Object.assign( Object.create( Pass.prototype ), { | |||
| this.renderTargetsHorizontal[ i ].setSize( resx, resy ); | |||
| this.renderTargetsVertical[ i ].setSize( resx, resy ); | |||
|
|
|||
| this.separableBlurMaterials[ i ].uniforms[ "texSize" ].value = new Vector2( resx, resy ); | |||
| this.separableBlurMaterials[ i ].uniforms[ "texSize" ].value = new Vector2( resx / window.devicePixelRatio, resy / window.devicePixelRatio ); | |||
There was a problem hiding this comment.
There's no guarantee of what a user has set the pixel ratio of the effect composer to or that they have set it at all in which case the effect will be inconsistent again so this doesn't feel like an adequate fix. I often change the pixel ratio dynamically, as well, depending on the run time performance of my application.
The postprocessing package deals with this by using a fixed render target resolution so the effect is consistent relative to the screen size. You can see an example of it in the blur demo. Something similar may be a better solution here.
There was a problem hiding this comment.
I often change the pixel ratio dynamically, as well, depending on the run time performance of my application.
Hello @gkjohnson, thank you for your information, I'll research it. But why you change window.devicePixelRatio? Although it can be changed, but I think this parameter should be a parameter of a physical device and should not be changed. If really needed, can other new variable be used instead?
There was a problem hiding this comment.
I think he meant he changes the value on the composer, so you cant assume that it equals to window.devicePixelRatio that you used here
There was a problem hiding this comment.
@makc OK, thanks! I'll research more.
There was a problem hiding this comment.
I think he meant he changes the value on the composer, so you cant assume that it equals to window.devicePixelRatio that you used here
That's right -- I adjust it based on performance because it can have a significant impact. On some devices the pixel ratio is set to 3 or 4 so lowering it can help a lot without really sacrificing visuals.
There was a problem hiding this comment.
@Mugen87 I found that, at least for this UnrealBloomPass, resolution is only used for initialize resx and resy, change resolution afterwards will no any effect.
And the initialized values will be overwritten by setSize() immediately, because when composer.addPass( bloomPass ); is called, composer will automatically call setSize().
However, uniform naming must be done, I'm trying.
There was a problem hiding this comment.
I added a this.resizer to keep this.resolution untouched, so will not cause naming problem.
There was a problem hiding this comment.
I think that the initialization code in the constructor should also be deleted, because they will be overwritten by the code in setSize() immediately, there is no need to exist, and the code complexity can be simplified.
There was a problem hiding this comment.
@gkjohnson I found that this Resizer.js solution is fixed height, which will be very wasteful of performance under small size and wide-screen-ratio.
There was a problem hiding this comment.
I think using Resizer.js will introduce too much complexity, and it also has performance problems under low resolution, so I don't want to use Resizer.js.
How about let EffectCompoer pass pixelRatio to Passs through setSize()?
Thus, Pass can aware of the current pixelRatio, even if it is manually modified?
gonnavis
force-pushed
the
UnifyPcMobUnrealBloomPass
branch
from
32387a0 to
1ed1640
Compare
gonnavis
force-pushed
the
UnifyPcMobUnrealBloomPass
branch
from
1ed1640 to
60184fb
Compare
gonnavis
force-pushed
the
UnifyPcMobUnrealBloomPass
branch
from
1a299a8 to
0096d29
Compare
gonnavis
force-pushed
the
UnifyPcMobUnrealBloomPass
branch
from
f769a55 to
9d4917f
Compare
gonnavis
force-pushed
the
UnifyPcMobUnrealBloomPass
branch
from
8367fcc to
2df2213
Compare
|
Closing, see #20798 (comment). |
|
@Mugen87 Hello, is this link wrong? I see the third page of pr list, not a specific pr. |
|
Link updated. |
Related issue: -
Description
At present, the display effect of UnrealBloomPass on pc and mob is very inconsistent.
To make the difference more obvious, I changed the geometry based on the current example.
https://raw.githack.com/gonnavis/three.js/d588f473a695cf1409d5f2de46835823795b6929/examples/webgl_postprocessing_unreal_bloom.html
PC current:
MOB current:
I found that
EffectComposerautomatically multiplieddevicePixelRatiaowhensetSize()caused this problem. AndEffectComposeris very correct in doing so, fabric.js and create.js will automatically multiply the canvas size bydevicePixelRatioto ensure high image quality on mobile devices too. So we can't change the code of EffectComposer.Therefore, I think the shader author must consider the influence of
devicePixelRatioby self.After this pr:
https://raw.githack.com/gonnavis/three.js/0345cba81af86885f75bbf988d5aeb474248ce52/examples/webgl_postprocessing_unreal_bloom.html
PC after:
MOB after:
