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

Fix sun positioning in sky shader example #21575

Merged
merged 3 commits into from
Apr 20, 2021
Merged

Conversation

sirxemic
Copy link
Contributor

@sirxemic sirxemic commented Apr 5, 2021

I noticed while experimenting with the example that changing the azimuth would influence the inclination as well. Turns out the sun position was calculated incorrectly from these two parameters.

I also changed the UI values for the inclination such that 0 means no inclination, which I thought makes more sense.

@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2021

@mrdoob mrdoob added this to the r128 milestone Apr 5, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2021

Nice catch!

I guess it applies to the ocean demo too?
https://threejs.org/examples/?q=ocean#webgl_shaders_ocean

@sirxemic
Copy link
Contributor Author

Indeed. Fixed it for that example as well just now.

@@ -75,7 +75,7 @@
gui.add( effectController, "rayleigh", 0.0, 4, 0.001 ).onChange( guiChanged );
gui.add( effectController, "mieCoefficient", 0.0, 0.1, 0.001 ).onChange( guiChanged );
gui.add( effectController, "mieDirectionalG", 0.0, 1, 0.001 ).onChange( guiChanged );
gui.add( effectController, "inclination", 0, 1, 0.0001 ).onChange( guiChanged );
gui.add( effectController, "inclination", -1, 1, 0.0001 ).onChange( guiChanged );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does a negative inclination make sense? I think the range should be like in webgl_shaders_ocean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the proper term is solar elevation. (The term solar altitude is also used, but that can be confusing for obvious reasons.)

Solar elevation is in degrees, and can be negative (at twilight, for example). A proper model should handle that case.

Solar azimuth is also in degrees.

/ping @zz85

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does a negative inclination make sense?

I'm asking this because changing to a negative value has no visual effect in the demo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any value smaller than -0.0257 has no visual effect, to be more precise. Values lower than that may make no difference visually, but it does allow the user to see the effect of when the sun is at the opposite side of the world.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that scene goes dark when the sun hits the horizon is either an error in the model, or an error in the implementation of it. That can be saved for another time if someone is interested in pursuing it.

Copy link
Collaborator

@Mugen87 Mugen87 Apr 19, 2021

Choose a reason for hiding this comment

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

@WestLangley What range of angles would you suggest for azimuth and elevation?

Looking at the Wikipedia article it seems the angles are always positive. Assuming we would have [-90, 90] for elevation. What part of the hemisphere would be expressed in negative angles?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Mugen87

From https://www.esrl.noaa.gov/gmd/grad/solcalc/azel.html

Azimuth is measured in degrees clockwise from north. Elevation is measured in degrees up from the horizon.

I think it is OK for azimuth to be in [ -180, 180 ] for the demo, or [ 0, 360 ] -- whatever you prefer. We do not have a concept of 'north'.

Since the model goes black below the horizon, [ 0, 90 ] for elevation in the demo seems appropriate to me. (Negative values, if supported by the model, would correspond to pre-dawn or post-sunset, when the sun is just below the horizon.

JMHO. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for spotting this, and for interesting discussion. agreed with the points @WestLangley brought up - azimuth and elevation make sense, and so is having some notion of sun below the horizon for twilight. the model used in the shader have some limited support for negative elevation, which have already been pointed up here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

the model used in the shader have some limited support for negative elevation

Hi @zz85! Very limited -- and not to this extent, I think.

@mrdoob mrdoob merged commit a511e81 into mrdoob:dev Apr 20, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 20, 2021

Thanks!

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