SSAO delayed resize fix #4342

GSterbrant · 2022-06-20T10:09:52Z

Fixes #4239

Removed the resize timeouts. Added a general resize callback to the post effects such that they can handle resize.

Bloom needs such a callback to adapt it's internal targets (which should save performance when viewport is small).

Removed the resize timeout in the post effects queue. Removed the resizecanvas listener from the SSAO effect.

mvaligursky · 2022-06-20T10:13:55Z

Please add the handling of this for the bloom as well.

src/framework/components/camera/post-effect-queue.js

scripts/posteffects/posteffect-ssao.js

…I was working on.

Also fixed missing line in the vertex shader.

mvaligursky · 2022-06-20T11:40:38Z

src/framework/components/camera/post-effect-queue.js

@@ -76,10 +72,6 @@ class PostEffectQueue {
         */
        this.resizeLast = 0;


is this.resizeLast still needed?

mvaligursky · 2022-06-21T14:29:18Z

src/framework/components/camera/post-effect-queue.js

@@ -404,6 +368,8 @@ class PostEffectQueue {

        for (let i = 0, len = effects.length; i < len; i++) {
            const fx = effects[i];
+            if (fx.effect.resize !== undefined)


Suggested change

if (fx.effect.resize !== undefined)

if (fx.effect.resize)

mvaligursky · 2022-06-21T14:30:52Z

scripts/posteffects/posteffect-bloom.js

+BloomEffect.prototype.resize = function (target) {
+
+    var width, height;
+    if (target == null) {


Suggested change

if (target == null) {

if (target === null) {

mvaligursky · 2022-06-21T14:31:18Z

scripts/posteffects/posteffect-bloom.js

+        height = target.colorBuffer.height;
+    }
+
+    if (width == this.width && height == this.height)


Suggested change

if (width == this.width && height == this.height)

if (width === this.width && height === this.height)

mvaligursky · 2022-06-21T14:31:53Z

scripts/posteffects/posteffect-bloom.js

+        return;
+
+    if (this.targets) {
+        for (let i = 0; i < this.targets.length; i++) {


cannot use let in ES5, only var

mvaligursky · 2022-06-21T14:34:41Z

scripts/posteffects/posteffect-ssao.js

-    var width = Math.ceil(this.device.width / this.device.maxPixelRatio / this.downscale);
-    var height = Math.ceil(this.device.height / this.device.maxPixelRatio / this.downscale);
+    var width, height;
+    if (target == null) {


Suggested change

if (target == null) {

if (target === null) {

mvaligursky · 2022-06-21T14:35:32Z

scripts/posteffects/posteffect-ssao.js

+    } else {
+        width = Math.ceil(target.colorBuffer.width / this.device.maxPixelRatio / this.downscale);
+        height = Math.ceil(target.colorBuffer.height / this.device.maxPixelRatio / this.downscale);
+    }

    // If no change, skip resize
    if (width == this.width && height == this.height)


could you please use === here as well

mvaligursky

approving with some minor comments (please fix before the merge)

lucaheft · 2022-07-04T08:00:15Z

scripts/posteffects/posteffect-ssao.js

-    var width = Math.ceil(this.device.width / this.device.maxPixelRatio / this.downscale);
-    var height = Math.ceil(this.device.height / this.device.maxPixelRatio / this.downscale);
+    var width, height;
+    if (target === null) {


@GSterbrant If the target is undefined this method will error (happens in line 561).
I guess it should be checked if the target is actually defined and instead of passing null everywhere, just call the function without arguments?

lucaheft · 2022-07-04T08:00:25Z

scripts/posteffects/posteffect-ssao.js

@@ -577,11 +561,6 @@ SSAO.prototype.initialize = function () {
        this.effect.resize();


In my opinion this resize should also happen if the effect is not enabled. Else we can have this situation:
User disables the effect -> User changes the downscale -> User enables the effect
Now we have the situation, that the render targets did not change in size although downscale was changed

lucaheft · 2022-07-04T09:34:07Z

Also the new resize function should be added to the pc.PostEffect docs.
(https://developer.playcanvas.com/en/api/pc.PostEffect.html)

mvaligursky · 2022-07-04T09:44:56Z

Perhaps the resize function should be called internally within the effect, and not from the engine (post-effect-queue). Inside the render call, the effect should check the sizes of its internal buffers vs the source / target buffer it has, and rescale those internal buffers as needed.

GSterbrant · 2022-07-04T10:01:43Z

The effect itself doesn't know about the input target and has to defer to the graphics device for dimensions. The post effects queue knows about the input targets, and can therefore forward it to the effect when it's enabled/disabled.

Wouldn't it be better if we can avoid dealing with resize related code if we're not resizing?

mvaligursky · 2022-07-04T10:07:18Z

the render function of the effect knows input and output
render: function (inputTarget, outputTarget, rect)

handling size compare is not very expensive, we handle it in few places like this. It's simpler for the user / creator of the effect.

engine/src/framework/scene-grab.js

Lines 156 to 158 in 24f325f

    
           if (this.colorRenderTarget?.width !== device.width || this.colorRenderTarget?.height !== device.height) { 
        
               self.releaseRenderTarget(this.colorRenderTarget); 
        
               this.colorRenderTarget = self.allocateRenderTarget(this.colorRenderTarget, device, this.colorFormat, false, true, false);

mvaligursky · 2022-07-04T10:08:30Z

This way you also avoid to maintain / handle resizing while the effect / camera is disabled and similar. Each time you render, you make sure targets are the right size.

lucaheft · 2022-07-04T10:20:49Z

I like that approach. This way the resizing can be handled on a per post-effect instance.
The post-effect-queue shouldnt handle it anyway, it's just a simple queue isn't it?

lucaheft · 2022-07-04T10:40:00Z

Also what i noticed, in the post-effect-queue the width and height is calculated like this:

engine/src/framework/components/camera/post-effect-queue.js

Lines 364 to 365 in 24f325f

    
           const desiredWidth = Math.floor(rect.z * this.app.graphicsDevice.width * this.renderTargetScale); 
        
           const desiredHeight = Math.floor(rect.w * this.app.graphicsDevice.height * this.renderTargetScale);

In the bloom post effect like this:

engine/scripts/posteffects/posteffect-bloom.js

Lines 193 to 194 in 24f325f

    
           width = this.device.width; 
        
           height = this.device.height;

And in ssao like this:

engine/scripts/posteffects/posteffect-ssao.js

Lines 391 to 392 in 24f325f

    
           width = Math.ceil(this.device.width / this.device.maxPixelRatio / this.downscale); 
        
           height = Math.ceil(this.device.height / this.device.maxPixelRatio / this.downscale);

Shouldnt those calculations all be the same? Except for the downscale in the ssao effect.

GSterbrant · 2022-07-04T10:46:21Z

The issue of having to make this calculation at all is due to the fact that post effects are setup prior to knowing the input target. This will be fixed in the PR I am working on addressing these issues :).

The post effect queue calculates the size of the internal targets, and really the effects should just read those values. The device dimensions shouldn't be used at all.

GSterbrant · 2022-07-04T10:50:51Z

I've made a PR here for these issues: #4397

GSterbrant added 2 commits June 20, 2022 11:06

Removed repetition of vertex shader declaration

a05cb36

Added a resize callback on post effects in case they define it.

33caf3c

Removed the resize timeout in the post effects queue. Removed the resizecanvas listener from the SSAO effect.

GSterbrant requested review from slimbuck and mvaligursky June 20, 2022 10:09

Lint fix.

f1add8a

mvaligursky reviewed Jun 20, 2022

View reviewed changes

src/framework/components/camera/post-effect-queue.js Outdated Show resolved Hide resolved

mvaligursky reviewed Jun 20, 2022

View reviewed changes

src/framework/components/camera/post-effect-queue.js Outdated Show resolved Hide resolved

slimbuck reviewed Jun 20, 2022

View reviewed changes

scripts/posteffects/posteffect-ssao.js Outdated Show resolved Hide resolved

GSterbrant added 4 commits June 20, 2022 11:22

This shader was wrong, accidentally carried over from another branch …

edae26d

…I was working on.

Resize accepts input target for dimensions.

c24ff1b

Also fixed missing line in the vertex shader.

Bloom responds to downscaling.

62d0699

Fix missing semicolon.

5a65a98

mvaligursky reviewed Jun 20, 2022

View reviewed changes

GSterbrant added 2 commits June 20, 2022 12:48

Removed dead code.

75849a9

Deleted unused import.

9717401

GSterbrant requested review from slimbuck and mvaligursky June 21, 2022 13:00

mvaligursky reviewed Jun 21, 2022

View reviewed changes

mvaligursky approved these changes Jun 21, 2022

View reviewed changes

GSterbrant added 2 commits June 21, 2022 15:52

Changed equality comparisons.

862dd9d

Lint

5090c64

GSterbrant merged commit abb890f into main Jun 21, 2022

GSterbrant deleted the gsterbrant_ssao_resize_fix branch June 21, 2022 15:12

lucaheft reviewed Jul 4, 2022

View reviewed changes

GSterbrant self-assigned this Jul 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSAO delayed resize fix #4342

SSAO delayed resize fix #4342

GSterbrant commented Jun 20, 2022

mvaligursky commented Jun 20, 2022

mvaligursky Jun 20, 2022

mvaligursky Jun 21, 2022

mvaligursky Jun 21, 2022

mvaligursky Jun 21, 2022

mvaligursky Jun 21, 2022

mvaligursky Jun 21, 2022

mvaligursky Jun 21, 2022

mvaligursky left a comment

lucaheft Jul 4, 2022

lucaheft Jul 4, 2022

lucaheft commented Jul 4, 2022

mvaligursky commented Jul 4, 2022

GSterbrant commented Jul 4, 2022

mvaligursky commented Jul 4, 2022

mvaligursky commented Jul 4, 2022

lucaheft commented Jul 4, 2022

lucaheft commented Jul 4, 2022

GSterbrant commented Jul 4, 2022

GSterbrant commented Jul 4, 2022 •

edited

		@@ -76,10 +72,6 @@ class PostEffectQueue {
		*/
		this.resizeLast = 0;

	if (width == this.width && height == this.height)
	if (width === this.width && height === this.height)

		@@ -577,11 +561,6 @@ SSAO.prototype.initialize = function () {
		this.effect.resize();

SSAO delayed resize fix #4342

SSAO delayed resize fix #4342

Conversation

GSterbrant commented Jun 20, 2022

mvaligursky commented Jun 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvaligursky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaheft commented Jul 4, 2022

mvaligursky commented Jul 4, 2022

GSterbrant commented Jul 4, 2022

mvaligursky commented Jul 4, 2022

mvaligursky commented Jul 4, 2022

lucaheft commented Jul 4, 2022

lucaheft commented Jul 4, 2022

GSterbrant commented Jul 4, 2022

GSterbrant commented Jul 4, 2022 • edited

GSterbrant commented Jul 4, 2022 •

edited