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

SSRPass: Change surfDist and infiniteThick behavior. #21539

Merged
merged 6 commits into from
Apr 15, 2021

Conversation

gonnavis
Copy link
Contributor

@gonnavis gonnavis commented Mar 29, 2021

Inspired by Blender Eevee SSR and SSRrPass's infiniteThick, changed SSRPass's infiniteThick behavior.

Demo

before vs after vs blender
image

---EDIT---
Newest Demo of this pr
image

@gonnavis gonnavis force-pushed the SSRPassChangeInfiniteThickBehaviorPr branch from 871be05 to c5a1418 Compare March 29, 2021 11:22
@gonnavis gonnavis changed the title SSRPass: Change infiniteThick behaviorPr. SSRPass: Change infiniteThick behavior. Mar 29, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 30, 2021

That's an strange effect 🤔

@gonnavis
Copy link
Contributor Author

gonnavis commented Mar 30, 2021

Yes, the parts far away from the reflection point are very strange, such as the mouth and the tip of the tail.

Many pros ( green ) and many cons ( red ). ( yellow: not sure pro or con )
image

I'll try to implement distanceAttenuation for this effect too, may can improve the visual effect.

@gkjohnson
Copy link
Collaborator

In my opinion "infinitely thick" shouldn't be the default in the demo because I agree it causes some odd artifacts. Instead I think the demo should just allow for increasing the apparent "thickness" of the ray marched buffer (called "surfDist" in the example page) to a higher value. And in fact setting it to a very high value should have the same effect as setting the thickness to "infinite".

Two more comments related to thickness:

  • It's not clear to me what "thickTolerance" does in the demo. I expected it to do what "surfDist" seems to be doing.
  • When adjusting "surfDist" it looks as though the surface is being thickened both behind and in front of the depth buffer being marched against. Instead I think it should just increase the thickness behind because we know where the front surface is. You just need thickness in order to "guess" where the back surface is. This change should also prevent the model from jumping positions when toggling the ground reflector on and off when surfdist is set to a larger value.

@gonnavis
Copy link
Contributor Author

gonnavis commented Mar 31, 2021

Thanks @gkjohnson !

I also have some similar optimization ideas, but they are rather vague. Your suggestions make me more clear about what to do.

Should let surfDist just affect the behind and merge the concept with thickTolerance ( which seems has problem now and I've not use it for a long time ) .

I'll do this first these days.

@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 1, 2021

Demo
image

Instead I think the demo should just allow for increasing the apparent "thickness" of the ray marched buffer (called "surfDist" in the example page) to a higher value. And in fact setting it to a very high value should have the same effect as setting the thickness to "infinite".
When adjusting "surfDist" it looks as though the surface is being thickened both behind and in front of the depth buffer being marched against. Instead I think it should just increase the thickness behind because we know where the front surface is. You just need thickness in order to "guess" where the back surface is.

Let surfDist only affect behind, and thus it is no longer surface, so renamed surfDist to thickness.
Now it's a very useful paramter to tweak to meet the needs of user's specific scene, and may can set each object seperately by the same way as #21487.

It's not clear to me what "thickTolerance" does in the demo. I expected it to do what "surfDist" seems to be doing.

Removed thickTolerance, because now only thinkness is sufficient.

This change should also prevent the model from jumping positions when toggling the ground reflector on and off when surfdist is set to a larger value.

Realy solved the main reason of this issue, still a little bias need to investigate.

reflection gaps problem on objects when camera near ground.

Suprisingly also partly solved this issue!

image

EDIT: Introduced a new problem if only SSR😂, fine with groundReflector.
image

@gonnavis gonnavis force-pushed the SSRPassChangeInfiniteThickBehaviorPr branch from 43270be to 1483183 Compare April 1, 2021 10:00
@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 1, 2021

But still some periodic artifacts I'm trying to solve.
image

And, is the breaking change which renaming exposed parameter from surfDist to thickness acceptable?

@gonnavis gonnavis changed the title SSRPass: Change infiniteThick behavior. SSRPass: Change surfDist and infiniteThick behavior. Apr 1, 2021
@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 2, 2021

Demo

All use NearestFilter instead of LinearFilter solved the periodic artifacts, now looking better.
image

The new introduced problem by thickness still exists but looking better too.
image

@mrdoob mrdoob added this to the r128 milestone Apr 3, 2021
@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 15, 2021

Found that surfDist ( now named as minThickness ) can be auto-calculated, don't need surfDist parameter anymore.

So in the end, not rename surfDist to thickness, but delete surfDist and add thickness.
They have different meaning and usage,
surfDist ( now named as minThickness ) is used for the min hit-test distance at that depth,
thickness is used for manually tweaking.

@gonnavis gonnavis marked this pull request as ready for review April 15, 2021 16:55
@mrdoob mrdoob merged commit 4bd5270 into mrdoob:dev Apr 15, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 15, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Apr 16, 2021

I'm not 100% sure but it seems like the changes broke the e2e tests...

https://github.com/mrdoob/three.js/runs/2358873732?check_suite_focus=true

Screenshot 2021-04-16 at 10 52 06

@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 16, 2021

When I execute npm run make-screenshot webgl_postprocessing_ssr locally, there is a high probability that will get stuck without returning, but sometimes will succeed.

After submitting it yesterday, e2e test has also been in the checking state for a very long time, and was discovered that failed today.🤔

I'm trying change the code to let local always success.

@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 16, 2021

Also found that when run npm run make-screenshot webgl_postprocessing_ssr, if get stuck, there's a very high CPU usage ,
and does not stop after terminated, left a running maybe headless chrome thread.

But works well in browsers ( Win10 chrome, Android Chrome, IOS safari ).

@mrdoob
Copy link
Owner

mrdoob commented Apr 16, 2021

This may be a SwiftShader issue?

@c0d1f1ed we use SwiftShader in our e2e setup and the changes in this PR made this example hang the process:

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_postprocessing_ssr.html

@gonnavis
Copy link
Contributor Author

gonnavis commented Apr 16, 2021

It's really hard to debug because this is a probability issue.
In the end I found after revert this commit will ok.

And found the main reason is

if(viewReflectRayZ-sD>vZ) continue;

changed to

if(viewReflectRayZ>vZ) continue;

I tried

if(viewReflectRayZ-.001>vZ) continue;

but still hang, and here not need variable anymore.

So tried

if(viewReflectRayZ<=vZ){
    bool hit;
    #ifdef INFINITE_THICK...
    #else...
    #endif...

    if(hit){...
    }
}

then OK.

@gonnavis
Copy link
Contributor Author

Seems continue commit to this merged pr has no effects.
Made a new pr and all checks ok at there.

@c0d1f1ed
Copy link

@mrdoob Thanks for alerting me about this. The Legacy SwiftShader OpenGL ES implementation unfortunately is known to have some bugs related to control flow in shaders. We're working on replacing it with the new SwiftShader Vulkan implementation, with the ANGLE project providing OpenGL ES compatibility for WebGL. This combo has no known shader bugs, so I'm hopeful it will also fix this issue.

Is there a URL for the demo of the old PR that we can try?

@mrdoob
Copy link
Owner

mrdoob commented Apr 19, 2021

I think this URL should work:
https://raw.githack.com/mrdoob/three.js/4bd5270bf0221dd4fd5f9372ae216f0ca7d6009a/examples/webgl_postprocessing_ssr.html

@c0d1f1ed
Copy link

I think this URL should work:
https://raw.githack.com/mrdoob/three.js/4bd5270bf0221dd4fd5f9372ae216f0ca7d6009a/examples/webgl_postprocessing_ssr.html

I can confirm that with Legacy SwiftShader, the page freezes when enabling the InfiniteThick feature. There also didn't appear to be any SSR effect. With the future "SwANGLE" solution, the page doesn't freeze and there's SSR going on.

There is some other periodic artifact which appears to be either some geometry going off into infinity, or some of the reflection bleeding where it shouldn't. We'll try to look into that.

@joshuaellis joshuaellis mentioned this pull request Apr 24, 2021
6 tasks
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.

None yet

4 participants