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

Water2 fragment shader does not support Z-up WCS #17390

Open
3 tasks done
viktorpts opened this issue Aug 30, 2019 · 4 comments
Open
3 tasks done

Water2 fragment shader does not support Z-up WCS #17390

viktorpts opened this issue Aug 30, 2019 · 4 comments
Labels

Comments

@viktorpts
Copy link

Description of the problem

The calculation which blends reflection and refraction fragment color assumes a default Y-up coordinate system to determine the blend amount. As a result, the transition between reflectance and refractance in a Z-up scene (or if the water surface doesn't lie in the xz-plane) happens while the camera is orbited around the scene, instead of while being tilted up and down. I was able to adjust the shader by trial and error, adding the following to line 339 of examples/jsm/objects/Water2.js:

vec3 adjustedEye = vec3(toEye.x, toEye.z, toEye.y);
float theta = max( dot( adjustedEye, normal ), 0.0 );

Which simply swaps the z and y coordinates for this calculation only. It would be nice if the shader works for any camera-up and plane orientation by taking into account the normal of the surface to which it is applied, however my lack of GLSL knowledge prevents me from coming up with a solution.

Three.js version
  • r106
Browser
  • All of them
OS
  • All of them
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 30, 2019

In general, we do not ensure that the example code like shaders work in all possible camera configurations. In most cases, it's up to the application code to make the necessary adjustments.

What are the reasons for using z-up anyway? I think in many cases you can make your life easier by stick to the default y-up convention.

@Mugen87 Mugen87 added the Addons label Aug 30, 2019
@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 30, 2019

I stumbled upon this line while investigating my own Water2 problem in #17251. Wonder if it is relevant here, as it explicitly sets the up direction of the virtual camera to y-up regardless of the up direction of the main camera (there is no matching line in Refractor.js, BTW):

virtualCamera.up.set( 0, 1, 0 );

@viktorpts
Copy link
Author

In general, we do not ensure that the example code like shaders work in all possible camera configurations. In most cases, it's up to the application code to make the necessary adjustments.

The discussion in this issue tracker is quite the resource for problem-solving (for me, at least), so I figured my solution can be used as reference by anyone who encounters a similar problem and searches for it online or in this repo, even if the issue is closed.

What are the reasons for using z-up anyway? I think in many cases you can make your life easier by stick to the default y-up convention.

It's not just the world orientation, if you rotate the surface with the shader on the x- or z-axis, it wont work correctly. In the concrete case I'm dealing with an app that has a 2D world rendered in 3D, so the third coordinate is redundant and I figured I'd save some mental effort by keeping everything in (x,y). It's also the convention for many engineering and architectural applications which I'm used to, so this also helps.

I stumbled upon this line while investigating my own Water2 problem in #17251. Wonder if it is relevant here, as it explicitly sets the up direction of the virtual camera to y-up regardless of the up direction of the main camera (there is no matching line in Refractor.js, BTW):

This has no effect, as it is overwritten further in the code - everything is copied from the scene and camera with a transformation applied, using the reflecting surface's normal as reference, so the WCS-up is irrelevant - I even rendered the output to a separate canvas to make sure. It's only the mixing shader that assumes y-distance == distance from the surface.

@FishOrBear
Copy link
Contributor

I also use z-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants