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 disappearing terrain and sky #3032

Merged
merged 11 commits into from
Sep 16, 2015
Merged

Fix disappearing terrain and sky #3032

merged 11 commits into from
Sep 16, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Sep 15, 2015

  • Fixed the sky atmosphere fragment shader that was causing the atmosphere to disappear when near or under the surface of the ellipsoid.
  • Fixed occlusion culling to only test occlusion when the camera is not under the surface of the ellipsoid.

Fixes both #2271 and #2415.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 15, 2015

The easiest way to test the sky atmosphere is to set:

scene.screenSpaceCameraController.enableCollisionDetection = false;

and rotate around one of the 3D models positioned on the ground.

@bagnell bagnell changed the title Fix disappearing sky Fix disappearing terrain and sky Sep 15, 2015
@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

I get this atmosphere on Mac in Chrome and Firefox:

image

Here it is without the globe:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

Looks OK near the ground:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

When we fix this, can we also add a comment to SkyAtmosphereFS.glsl about the workaround/fix?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

Can we also change the default for screenSpaceCameraController.minimumZoomDistance in this PR? What do you think? 1m? 0.1m?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

Looks good.

The only other issue is zoom to the dead sea and quickly use the middle mouse button to take a horizon view - the camera will start moving backwards. If this is not an easy fix, submit a new issue.

…a is below the ellipsoid surface, switch to a ray sphere intersection test.
@bagnell
Copy link
Contributor Author

bagnell commented Sep 16, 2015

@pjcozzi This is ready for another look.

The camera problem you mentioned is intentional. The camera slides over terrain when a collision is detected to prevent it from getting stuck.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

Do you get these test failures:

Scene/ScreenSpaceCameraController camera does go below the terrain in 3D when collision detection is disabled
Expected 1.000000000846144 to be less than 1.
Error: Expected 1.000000000846144 to be less than 1.
    at stack (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1475:17)
    at buildExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1445:14)
    at Spec.Env.expectationResultFactory (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:584:18)
    at Spec.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:332:34)
    at Expectation.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:528:21)
    at Expectation.toBeLessThan (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1399:12)
    at http://localhost:8080/Specs/Scene/ScreenSpaceCameraControllerSpec.js:1059:52
    at Object.<anonymous> (http://localhost:8080/Specs/spec-main.js:137:30)
    at attemptAsync (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
    at QueueRunner.run (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1746:26)
Scene/ScreenSpaceCameraController camera does go below the terrain in CV when collision detection is disabled
Expected 1.000000000846144 to be less than 1.
Error: Expected 1.000000000846144 to be less than 1.
    at stack (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1475:17)
    at buildExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1445:14)
    at Spec.Env.expectationResultFactory (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:584:18)
    at Spec.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:332:34)
    at Expectation.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:528:21)
    at Expectation.toBeLessThan (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1399:12)
    at http://localhost:8080/Specs/Scene/ScreenSpaceCameraControllerSpec.js:1075:35
    at Object.<anonymous> (http://localhost:8080/Specs/spec-main.js:137:30)
    at attemptAsync (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
    at QueueRunner.run (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1746:26)
Scene/ScreenSpaceCameraController camera does not go below the terrain in CV with the transform set
Expected 0.9999999948034711 to be greater than or equal to 1.
Error: Expected 0.9999999948034711 to be greater than or equal to 1.
    at stack (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1475:17)
    at buildExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1445:14)
    at Spec.Env.expectationResultFactory (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:584:18)
    at Spec.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:332:34)
    at Expectation.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:528:21)
    at Expectation.toBeGreaterThanOrEqualTo (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1399:12)
    at http://localhost:8080/Specs/Scene/ScreenSpaceCameraControllerSpec.js:1101:37
    at Object.<anonymous> (http://localhost:8080/Specs/spec-main.js:137:30)
    at attemptAsync (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
    at QueueRunner.run (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1746:26)

@@ -155,7 +155,7 @@ define([
* @type {Number}
* @default 20.0
*/
this.minimumZoomDistance = 20.0;
this.minimumZoomDistance = 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention this in CHANGES.md.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 16, 2015

@pjcozzi The tests are fixed. This is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 16, 2015

Awesome.

pjcozzi added a commit that referenced this pull request Sep 16, 2015
@pjcozzi pjcozzi merged commit 0836231 into master Sep 16, 2015
@pjcozzi pjcozzi deleted the sky-bug branch September 16, 2015 20:48
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.

2 participants