-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
There was a problem hiding this 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
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. |
Today I've been experimenting with the 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 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: So the existing dynamicSSEDensity default of 0.00278 gives a fog distance of around 180m, that's pretty close to the camera. Meanwhile 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?) |
@@ -134,6 +134,10 @@ describe( | |||
function createTileset(url) { | |||
const options = { | |||
maximumScreenSpaceError: 0, | |||
// The camera is zoomed pretty far out for these tests, so |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This morning I set 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:
I also ran the performance tests again:
Raw data: Snapshot of testing HTML files: (these require a local release build of CesiumJS) performance-melbourne.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 |
I talked with @ggetz, a summary:
|
@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) Google Earth: 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. |
@ggetz this is ready for review again! |
packages/widgets/Source/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js
Show resolved
Hide resolved
There was a problem hiding this 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!
@ggetz I addressed your feedback! |
Awesome, thanks @ptrgags! |
Description
This PR:
tileset.dynamicScreenSpaceError
totrue
, enabling this optimization for lowering the resolution of tiles far away in horizon viewsCesium3DTilesetConstructor
to assign initial values based on constructor optionsdynamicScreenSpaceError*
parametersExample of it in action: local Sandcastle -- open up the
Update
section and adjust the SSE Density/Factor sliders: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:
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change