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

Fog #3154

Merged
merged 47 commits into from
Nov 9, 2015
Merged

Fog #3154

merged 47 commits into from
Nov 9, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Oct 30, 2015

  • Tweak fade distance and density
  • Change tile SSE when in fog
  • Use preprocessor instead of conditional when enabled.
  • Swap shader when a tile is or is not in fog
  • Performance
  • Tests
  • Reference doc including advice on the performance / visual quality trade-off
  • Update CHANGES.md

viewModel.endHeight = viewer.scene.fogEndHeight;

/*
Sandcastle.addToolbarMenu([{
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using the same views we used for OBBs: http://cesiumjs.org/2015/06/24/Oriented-Bounding-Boxes/

Instead of commenting them out, they could be in a drop down for future testing.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 1, 2015

Added a few items to the tasklist

  • Tests
  • Reference doc including advice on the performance / visual quality trade-off
  • Update CHANGES.md

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 1, 2015

@bagnell add any ideas we come up with that are outside the scope of this PR to the renderer roadmap, #3001.

var surfaceTile = tile.data;
if (frameState.fogEnabled) {
var distance = this.computeDistanceToTile(tile, frameState);
var scalar = distance * frameState.fogDensity;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the fog equation to a @private helper function. Keeping it in this file is probably fine for now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 2, 2015

Zoom in the part of the globe that is dark due to the ground atmosphere. Note how the fog blacks out the distant terrain.

With fog:

image

Without fog:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 2, 2015

How hard is it to use the ground atmosphere if the ray hits the ellipsoid, but the sky atmosphere if the ray doesn't?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 2, 2015

What are your stats for the default view in the fog Sandcastle example.

With ~3K resolution:

No fog:

Visited 751, Rendered: 498, Culled: 263, Max Depth: 20, Waiting for children: 0

Fog:

Visited 719, Rendered: 477, Culled: 251, Max Depth: 20, Waiting for children: 0

Not great.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 2, 2015

For the night ground atmosphere, we might want to use the negative sun vector or something. For example:

No fog:

image

Fog:

image

Note that for this view, fog is pretty useful.

No fog:

Visited 140, Rendered: 86, Culled: 78, Max Depth: 10, Waiting for children: 0

Fog:

Visited 100, Rendered: 63, Culled: 50, Max Depth: 10, Waiting for children: 0

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 2, 2015

Overall, I am not impressed with the number of tile reduced. I expected a lot more. Perhaps we can increase the start of the step function and decrease the completely-in-fog distance.

I also imagine the VMSSE-style SSE based on fog will be somewhat effective; however, it's effectiveness will depend on how many tiles are partially in fog. Above, I am suggesting to put more tiles completely in fog, but fewer partially in fog...

@mramato
Copy link
Contributor

mramato commented Nov 3, 2015

When this is done, it would also be interesting to see numbers for when maximumScreenSpaceError is set to 1 instead of it's current default of 2, since 1 is the value we want to be able to use for the default (but need to bring the number of tile requests down in order to do it).

As an aside, is the minimum value for maximumScreenSpaceError 1 or 0? I always assumed it was 1 but the doc doesn't specify.

@kring
Copy link
Member

kring commented Nov 3, 2015

As an aside, is the minimum value for maximumScreenSpaceError 1 or 0?

Any value > 0 is allowed. 0 makes no sense, but less than 1 is fine. It doesn't have to be an integer.

You can also think of it as a range. If your maximumScreenSpaceError is 2, your texels will be between 1 and 2 pixels on the screen. If maximumScreenSpaceError is 1, they'll be between 0.5 and 1 pixels on the screen. 1.5 might be a nice value for the default, giving you between 0.75 and 1.5 pixels.

@mramato
Copy link
Contributor

mramato commented Nov 3, 2015

Any value > 0 is allowed. 0 makes no sense, but less than 1 is fine. It doesn't have to be an integer.

I'll write up an issue to clarify the doc. We might also want to consider adding a get/set property to enforce non-zero values.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 6, 2015

Default density and SSE factor seem sensible. Increasing the SSE factor can result in significant savings (like another 33%), but, of course, hurts visual quality and makes popping more obvious when the camera moves.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 6, 2015

@bagnell can you do a quick sanity check for when the camera is really low or has a negative altitude?

For a horizon view in the dead see, without fog:

Visited 236, Rendered: 148, Culled: 118, Max Depth: 14, Waiting for children: 0

image

With fog:

Visited 47, Rendered: 21, Culled: 59, Max Depth: 14, Waiting for children: 0

image

That seems too aggressive.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 6, 2015

For a horizon view at a high altitude, I am seeing a lot of cracking between the globe and the sky atmosphere. It loads eventually, but I wonder if we are too aggressive at this altitude.

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 6, 2015

Wow, check out these stats for a horizon view near the ground in FL:

No fog:

Visited 352, Rendered: 226, Culled: 154, Max Depth: 18, Waiting for children: 0

Fog:

Visited 139, Rendered: 82, Culled: 91, Max Depth: 18, Waiting for children: 0

No difference in visual quality.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 6, 2015

For a horizon view at a high altitude, I am seeing a lot of cracking between the globe and the sky atmosphere. It loads eventually, but I wonder if we are too aggressive at this altitude.

Also follow the ISS or GeoEye satellite in the CZML Sandcastle example to see this a bit.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 6, 2015

How many tiles are saved for a horizon view like this in space? Perhaps we should just turn it off at this altitude.

image

@bagnell
Copy link
Contributor Author

bagnell commented Nov 6, 2015

@pjcozzi I addressed all of your comments. This is ready for another look.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2015

I get this exception when I open Hello World in Edge in this branch, but not master. Perhaps the other browsers are not checking the GLSL as carefully or is this a bug in Edge?

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2015

Actually, Firefox also doesn't like it:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2015

2D is fixed. Looks like fog is also disabled in Columbus view. Does it need to be? It appeared to work when I tested it last.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2015

Since we removed the discard statements for the atmosphere, the atmosphere can look a bit rough when waiting for the initial globe tiles to load:

image

We could just leave this or not draw the atmosphere until the globe renders something. I can't come up with a reason that drawing the atmosphere without the globe is useful.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2015

Also, for the blog post, can you include FPS (if you can get useful data) in addition to the number of tiles?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2015

Just those comments and this is ready!

@bagnell
Copy link
Contributor Author

bagnell commented Nov 9, 2015

@pjcozzi This is ready.

@mramato
Copy link
Contributor

mramato commented Nov 9, 2015

  1. Did we test this on mobile?
  2. Should we add a toggle fog button to the Terrain example? I think it would come in handy.
  3. Some views still look really washed out, take this shot with and without fog near San Francisco Bay. Maybe this is just the nature of fog? But it makes some views pretty bad.

Without
image

With
image

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 9, 2015

Tests look good now.

Did we test this on mobile?

Good idea.

Should we add a toggle fog button to the Terrain example? I think it would come in handy.

Also, a good idea.

Some views still look really washed out, take this shot with and without fog near San Francisco Bay. Maybe this is just the nature of fog? But it makes some views pretty bad.

This is OK with me. The less aggressive we make it, the less performance will improve. This was already tweaked quite a bit, and is a reasonable default. We can change it later if needed.

Let me know when this is ready.

@bagnell
Copy link
Contributor Author

bagnell commented Nov 9, 2015

@pjcozzi This is ready.

pjcozzi added a commit that referenced this pull request Nov 9, 2015
@pjcozzi pjcozzi merged commit ade6742 into master Nov 9, 2015
@pjcozzi pjcozzi deleted the fog branch November 9, 2015 21:02
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.

4 participants