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 : fixed MAX_STEP #21384

Merged
merged 4 commits into from
Mar 2, 2021
Merged

SSRPass : fixed MAX_STEP #21384

merged 4 commits into from
Mar 2, 2021

Conversation

ycw
Copy link
Contributor

@ycw ycw commented Mar 1, 2021

@ycw ycw changed the title fixed MAX_STEP SSRPass : fixed MAX_STEP Mar 1, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 1, 2021

/ping @gonnavis

@mrdoob mrdoob added this to the r127 milestone Mar 1, 2021
@gonnavis
Copy link
Contributor

gonnavis commented Mar 1, 2021

@ycw Thanks!

  1. There indeed that this.width and this.height should be used instead of window.innerXXX to keep code consistency and let the application level control the width and height.

  2. And I tested if MAX_STEP happens to be an integer, it will indeed cause shader errors.
    Could you please add toFixed(1) at here too?

    this.ssrMaterial.defines.MAX_STEP = Math.sqrt( width * width + height * height );

    I think that usually, the MAX_STEP here will overwrite the initial setting, so it is more important.

  3. EDIT: Oh, get it, What matters is the order, not the form. Don't mind the following question.
    Also out of curiosity, whether according to convention, this form

Object.assign( {}, SSRShader.defines, {
    MAX_STEP: abc,
}),

should be used instead of

Object.assign( {
    MAX_STEP: abc,
}, SSRShader.defines )

?

MAX_STEP: Math.sqrt( window.innerWidth * window.innerWidth + window.innerHeight * window.innerHeight )
}, SSRShader.defines ),
defines: Object.assign( {}, SSRShader.defines, {
MAX_STEP: Math.sqrt( this.width * this.width + this.height + this.height ).toFixed(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be multiplication sign here? this.height * this.height

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2021

How about doing this instead?

for(float i=0.0,l=float(MAX_STEP);i<l;i++){

@ycw
Copy link
Contributor Author

ycw commented Mar 1, 2021

Object.assign( {}, SSRShader.defines, {
    MAX_STEP: abc,
}),

should be used instead of

Object.assign( {
    MAX_STEP: abc,
}, SSRShader.defines )

?

const his = { x : 1 }
const her = { x : 2 }
Object.assign( his, her ) // { x : 2 } ( the 1st par is mutated )
his.x === 2 // true

implies SSRShader.defines.MAX_STEP (if any will replace MAX_STEP: abc .


btw, i also found one here

defines: Object.assign({
useDepthTexture: useDepthTexture
}, Reflector.ReflectorShader.defines),

@gonnavis
Copy link
Contributor

gonnavis commented Mar 1, 2021

How about doing this instead?

for(float i=0.0,l=float(MAX_STEP);i<l;i++){

@mrdoob @ycw Oh, this modification looks ok, but in actual use, I found that it will cause my browser and even the system to crash. 😨
I'm using Win10Pc and Chrome88.

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2021

@mrdoob @ycw Oh, this modification looks ok, but in actual use, I found that it will cause my browser and even the system to crash. 😨

That's very surprising... Could you test the PR in your system?

@gonnavis
Copy link
Contributor

gonnavis commented Mar 1, 2021

@mrdoob I have been keep testing the PR code and there is only a little need to be modified no big problem.
But with this change for(float i=0.0,l=float(MAX_STEP);i<l;i++){, I tested three times, two browser crashes and one system crash.

I remember that it may because glsl can't use dynamic variable in for loop. But why just crash not throw error, I have no idea~~.
I'm keep trying.

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2021

@gonnavis but for(float i=0.;i<float(MAX_STEP);i++){ works?

@gonnavis
Copy link
Contributor

gonnavis commented Mar 1, 2021

@gonnavis but for(float i=0.;i<float(MAX_STEP);i++){ works?

Yes, and

float max_step=float(MAX_STEP);
for(float i=0.;i<max_step;i++){
float max_step=50.;
for(float i=0.;i<max_step;i++){
uniform float max_step;
...
for(float i=0.;i<max_step;i++){

work too, very surprising.

I really remember that can't use variable and even can't use uniform in for loop, this is why I used defines for MAX_STEP. EDIT: This should be IOS and WebGL1 related.

@gonnavis
Copy link
Contributor

gonnavis commented Mar 1, 2021

@mrdoob @ycw Turned out that on IOS WebGL1 all these

float max_step=float(MAX_STEP);
for(float i=0.;i<max_step;i++){
float max_step=50.;
for(float i=0.;i<max_step;i++){
uniform float max_step;
...
for(float i=0.;i<max_step;i++){
for(float i=0.0,l=float(MAX_STEP);i<l;i++){

will cause error ERROR: 0:230: 'i' : Loop index cannot be compared with non-constant expression and loss the whole SSR effect.

Just for(float i=0.;i<float(MAX_STEP);i++){ works. Should because this just convert, not declare new variable.
But I am worried that there will be a little performance loss when writing this way.
So I personally suggest to use the toFixed method of @ycw at the beginning.

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2021

So I personally suggest to use the toFixed method of @ycw at the beginning.

I think that solution is fragile. We're likely to bump into that issue again if we rewrite.

I think using float(MAX_STEP) is more robust. At least the solution sits right where the problem starts.

@zeux Any insights about the best thing to do here? Is it a bad idea to cast a define right in the for loop?

for(float i=0.;i<float(MAX_STEP);i++){

@ycw
Copy link
Contributor Author

ycw commented Mar 1, 2021

@mrdoob @ycw Turned out that on IOS WebGL1 all these

will cause error ERROR: 0:230: 'i' : Loop index cannot be compared with non-constant expression and loss the whole SSR effect.

what if ..

const float l = float(MAX_STEP);
for (float i=0.; i<l;  ...

?

@gonnavis
Copy link
Contributor

gonnavis commented Mar 1, 2021

Excellent! it works on my iphone6sp WebGL1.

@zeux
Copy link
Contributor

zeux commented Mar 1, 2021

@mrdoob fwiw outside of WebGL & GLSLES I wouldn't worry about this; in WebGL1/ESSL specifically shaders have extra restrictions to enforce loop termination, see https://www.khronos.org/registry/webgl/specs/latest/1.0/#SUPPORTED_GLSL_CONSTRUCTS:

for loops must conform to the structural constraints in Appendix A.

https://www.khronos.org/registry/OpenGL/specs/es/2.0/GLSL_ES_Specification_1.00.pdf, Appendix A:

The for statement has the form:
for ( init-declaration ; condition ; expression ) statement
• init-declaration has the form:
type-specifier identifier = constant-expression
Consequently the loop variable cannot be a global variable.
• condition has the form
loop_index relational_operator constant_expression
where relational_operator is one of: > >= < <= == or !=

Noteworthy is that float(MAX_PASS) is a constant expression per ESSL specification. If the loop bound is a separate variable it must be declared with const float instead of float for the shader to conform. Once that's done, all of these forms should be equivalent in performance.

@gonnavis
Copy link
Contributor

gonnavis commented Mar 2, 2021

@ycw Many thanks!

Hello @Mugen87 @mrdoob , I think this pr is ok, and tested no problems.

@mrdoob
Copy link
Owner

mrdoob commented Mar 2, 2021

@zeux Thanks! 🙏

@mrdoob mrdoob merged commit 2b661c8 into mrdoob:dev Mar 2, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 2, 2021

Thanks!

@ycw ycw deleted the ssr branch March 2, 2021 11:35
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.

5 participants