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

Turn on dynamicScreenSpaceError by default #11718

Merged
merged 28 commits into from
Jan 4, 2024
Merged

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Dec 21, 2023

Description

This PR:

  • Sets tileset.dynamicScreenSpaceError to true, enabling this optimization for lowering the resolution of tiles far away in horizon views
  • Adds missing code in the Cesium3DTilesetConstructor to assign initial values based on constructor options
  • Fixes the 3D Tiles Inspector SSE Density slider (which wasn't updating the tileset), as this is helpful for testing.
  • Improves the documentation for dynamicScreenSpaceError* parameters

Example of it in action: local Sandcastle -- open up the Update section and adjust the SSE Density/Factor sliders:

image

Issue number and link

Fixes #6143
Fixes #11715
Fixes #11677

Testing plan

Choosing better defaults

Tomorrow I plan to focus on selecting better default values for these parameters.

I'm going to start with the performance testing page from #4196 (comment)

(here modified to use the copy of CesiumJS from npm build release and the local Cesium Dev server) - performance-local.html.txt.

This involves:

  1. Make more versions the perf testing page as needed to get a variety of datasets and camera views. I want to also include One World Terrain and Melbourne.
  2. I want to do a visual inspection using the 3D Tiles Inspector. How large can I increase the SSE factor/density without the quality loss being obvious? (Note: for Google P3DT, Google Earth serves as a visual benchmark)
  3. Once I narrow down the range of values, performance test a few more cases to verify that the changes give a performance boost everywhere.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code
  • Experiment with different SSE Factor and SSE Density settings to get the best performance without a noticeable reduction in quality (See Testing Plan above)

Copy link
Contributor Author

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

Self review, had a few comments about documentation

packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Dec 22, 2023

In setting up Maxar's One World Terrain base globe, I'm realizing that dynamic screen space error doesn't really impact this dataset much. Even the smallest tiles are relatively large, so to get a good horizon view you have to position the camera very far above the dataset (where the dynamicSSE has essentially no effect). So I'll try other datasets.

image

@ptrgags
Copy link
Contributor Author

ptrgags commented Dec 22, 2023

Today I've been experimenting with the dynamicScreenSpaceErrorFactor and dynamicScreenSpaceErrorDensity.

First I kept the density the same and adjusted the SSE Factor. 8 px seemed to be reasonable for most views I tried for the default density.

However, then I looked at the density value and realized that right now the fog is very close to the camera! And compared with scene.fog.density, it is a whole order of magnitude larger (closer to camera)

One conceptual thing I learned today. If you compare our fog function with a normal distribution, equate the exponents and solve for the standard deviation (i.e. distance to inflection point, you get: $$\sigma = \frac{1}{2\rho}$$ (where sigma is the stddev and rho is the density). This is handy for estimating how far away the fog's inflection point is from the camera.

So the existing dynamicSSEDensity default of 0.00278 gives a fog distance of around 180m, that's pretty close to the camera.

Meanwhile scene.fog.density of 2e-4 = 0.0002 gives a fog distance of 2.5 km. This seems more in the ballpark of a horizon view in reality (though it depends on the camera's elevation)

So I think the thing to do will be set the density default to match terrain (to avoid unnecessary loss in visual quality), and then adjust the SSE factor (maybe it can be made a bit higher than 8 to compensate performance wise?)

CHANGES.md Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
@@ -134,6 +134,10 @@ describe(
function createTileset(url) {
const options = {
maximumScreenSpaceError: 0,
// The camera is zoomed pretty far out for these tests, so
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wouldn't expect this to be needed. Why is this needed for 3D Tiles but not terrain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem seemed to be a race condition when waiting for the tileset to load in this particular test setup. I opened #11732 for this since it only seems to affect the unit tests, not the rendering itself.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 2, 2024

This morning I set dynamicScreenSpaceErrorDensity to 2e-4 like terrain, and then explored the SSE Factor setting for this configuration.

Considering that the "fog" is now a lot further into the distance, you can dial up the SSE Factor more without huge changes in visual appearance. For P3DT and Melbourne, 24 px seemed to be the highest I could go before it became noticeable, at least to my eyes (visual quality is subjective). The difference was most noticeable for the Tokyo aerial view, so here are some screenshots for comparison:

No dynamic SSE density 2e-4, SSE factor 16 density 2e-4, SSE factor 24
image image image

I also ran the performance tests again:

Configuration Average time to first load (s) % difference from no fog
No Fog 7.989 N/A
Fog with old defaults (density 0.00276, SSE Factor 4) 5.678 -28.93%
Fog with density 2e-4, SSE Factor 16 5.216 -34.71%
Fog with density 2e-4, SSE Factor 24 4.888 -38.82%

Raw data:
Performance Stats CSV

Snapshot of testing HTML files: (these require a local release build of CesiumJS)

performance-melbourne.html.txt
performance-p3dt.html.txt

So even though I had to reduce the density parameter, it's still better performance than the old defaults!

@ggetz what do you think, is 24 a reasonable default, or would it be better to pick a more conservative value (say 16 or less) for now until #11716 is implemented?

One other detail to put things in perspective: I notice that for terrain, the SSE Factor is equal to the maxSSE for terrain (2, compared with 3D Tiles which uses 16 as the default). This would mean 16 is roughly analogous. 24 is a bit more intense than that in comparison

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 2, 2024

I talked with @ggetz, a summary:

  • I should check my unit tests for race conditions, for some reason specs are failing on the command-line (both in CI and locally)
  • the SSE Factor value of 24 seems good
  • but to be extra sure, get a comparison screenshot between 3D Tiles P3DT and Google Earth to make sure it looks reasonable
  • Add unit tests for checking the default parameter values
  • For the changelog:
    • condense the entries about the defaults into one entry.
    • The changes about the inspector go in the widgets section, not the engine section

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 2, 2024

@ggetz Here's a visual comparison of CesiumJS and Google Earth. Again I used Tokyo as that had the biggest visual difference compared

CesiumJS (density 0.0002, SSE Factor 24)
image

Google Earth:

image

I find it hard to spot specific differences, as they're small and lost in the high-frequency details of the image. If anything, Google Earth looks a bit lower resolution in the distance.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 3, 2024

Strange, the picking test timing seems to be sensitive to the dynamic SSE feature, but I'm not sure why.

I'm looking at the test "Scene/Pick > pickFromRay > picks a tileset" (though there are other similar tests also affected).

The main difference I notice is the timing of loading the tile and content. For some reason tileset.tilesLoaded is true before the content is ready, and this leads to doing the picking call too early for the test to succeed. I'm not sure why this only seems to happen when dynamicScreenSpaceError is on.

Dynamic SSE off (test works) Dynamic SSE on (test fails)
image image

Some other observations:

  • When the picking happens, the camera is above the tileset, so dynamic SEE is computed as 0 (not a horizon shot)
  • however, the root tile has a transform that puts it somewhere in the US. This transform is replaced with a new ENU transform that puts it at cartographic(0, 0), but for the first few frames technically dynamic SSE is applied and acts like an almost-horizon view. So that tile gets culled until the new matrix is applied. I wonder if this is related.
  • the picking pass itself isn't affected by dynamic SSE, as picking uses an ortho frustum. the problem is that picking happens too early.

Sandcastle to show the test setup

@ptrgags ptrgags marked this pull request as ready for review January 3, 2024 22:02
@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 3, 2024

@ggetz this is ready for review again!

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @ptrgags! This is close!

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 4, 2024

@ggetz I addressed your feedback!

@ggetz
Copy link
Contributor

ggetz commented Jan 4, 2024

Awesome, thanks @ptrgags!

@ggetz ggetz merged commit 5d1b07c into main Jan 4, 2024
9 checks passed
@ggetz ggetz deleted the 11715-dyn-sse-by-default branch January 4, 2024 16:20
@ptrgags ptrgags mentioned this pull request Jan 5, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants