-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
3D Tiles - Dynamic Screen Space Error #4307
Conversation
@@ -131,7 +131,7 @@ define([ | |||
|
|||
// Fade fog in as the camera tilts toward the horizon. | |||
var positionNormal = Cartesian3.normalize(camera.positionWC, scratchPositionNormal); | |||
var dot = CesiumMath.clamp(Cartesian3.dot(camera.directionWC, positionNormal), 0.0, 1.0); | |||
var dot = Math.abs(Cartesian3.dot(camera.directionWC, positionNormal)); |
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.
I changed this because I noticed that any views that are looking from horizon-view to birds-eye-view would result in a negative dot product. So all views between horizon and birds-eye are considered to be in complete fog, but really it should adjust.
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.
By the way, this does not really affect anything in practice, so if anything we could just remove this code.
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.
Up to you and @bagnell
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.
You probably want to revert this. You can look up at terrain (try at Everest, it's pretty easy) in which case we want to clamp at zero.
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.
Actually, after talking offline. This will be fine. @lilleyse is going to update the tests.
@bagnell can you also take a quick look at this? |
@@ -70,6 +72,7 @@ define([ | |||
* @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] A 4x4 transformation matrix that transforms the tileset's root tile. | |||
* @param {Number} [options.maximumScreenSpaceError=16] The maximum screen-space error used to drive level-of-detail refinement. | |||
* @param {Boolean} [options.refineToVisible=false] Whether replacement refinement should refine when all visible children are ready. An experimental optimization. | |||
* @param {Boolean} [options.useDynamicScreenSpaceError=false] Reduce the screen space error for tiles that are further away from the camera. |
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.
We generally don't start variables with use
, e.g., above is just refineToVisible
, not useRefineToVisible
, so this could just be dynamicScreenSpaceError
or scaleScreenSpaceErrorByDistance
.
Just those comments. Will also test this with some datasets once the Sandcastle example has more options. |
When fog is used with a dynamic SSE for terrain, we also stop requesting tiles that are completely in fog. It is worth doing something similar here with a max distance? |
We could do that, but also the tiles in the distance should be pretty low res to begin with so there won't be that many requests saved. EDIT: it does depend on the dataset though. |
This widely depends on the tileset. Consider a root tile with 300 children, or just a uniform grid. |
Any concerns if I merge this into |
Nope, go ahead. |
My testing and the code look good. I still don't think the doc for I'll also hold off on merging until the local coordinate system change we discussed offline. |
Updated.
I could be wrong, but I think it's just doing the same thing as here, requesting less because it refines less. But the uniform grid type scenario for 3d tiles is definitely worth handling. Is it ok if I handle this in another PR? I also want to clean up some of the checks we do (check sse, check visibility, compute distance, sort by distance, check request volume) that we do in multiple places. |
* | ||
* @see Fog.density | ||
*/ | ||
this.dynamicScreenSpaceErrorDensity = 0.00278; |
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.
Any reason this and dynamicScreenSpaceErrorFactor
should not also be constructor arguments?
var root = tileset._root; | ||
var tileBoundingVolume = root.contentBoundingVolume; | ||
|
||
if (tileBoundingVolume instanceof TileBoundingRegion) { |
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.
Do you know how fast instanceof
is? If it isn't basically free we should not use it here.
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.
Ah, this is only for the root
tile. Probably not an issue, but we should still research.
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.
Yeah it's only once per update. According to jsPerf on my machine instanceof
can be done in 224,000 operations per second.
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.
That is actual kind of slow I bet compared to, for example, ===
with numbers. Regardless, it is fine here, of course.
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.
@lilleyse which test did you use? There's no way that's correct. I get 146,620,901 on this one https://jsperf.com/instanceof-vs-method?
The performance of instanceof depends on a lot of factors (the most significant of which is how far up the prototype chain you need to look.). But I wouldn't worry about it's usage unless you already identified a given function as a hotspot.
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.
I used this test: https://jsperf.com/instanceof-performance/2.
The code is not much of a hotspot, it is called once per frame.
} | ||
} | ||
|
||
// The range where the density starts to lessen. Start at the quarter height of the tileset. |
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.
Shouldn't this be a user option?
var scratchPosition = new Cartesian3(); | ||
var scratchDirection = new Cartesian3(); | ||
|
||
function updateDynamicScreenSpaceError(tileset, frameState) { |
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.
How is test coverage? Does the new test cover all the paths in this function?
I'll look at this |
Updated. I addressed the remaining points. I just have to add one more bounding volume test, and will update this issue when its ready (should be later today). |
CC @pmconne |
Looks good, let me know when the tests are finished. |
The last test is done now. |
Do these tests fail for you?
|
Ah those are caused by this change: #4307 (comment) Because of the old math, fog would be applied in bird's eye view, which is the view these tests use. But my change prevents fog from happening for those views. @bagnell can you check that my code change makes sense? |
@bagnell thoughts here? We want to merge this soon. |
Updated. Those tests pass now. |
👍 |
For #3241
Like terrain fog, this enables a dynamic screen space error for rendering lower detail tiles based on distance. I mainly tested this with replacement refinement tilesets and I see about 20% reduction in the number of visited tiles for street-level views for city-like tilesets and anywhere from 50% to 70% improvement for more terrain-like datasets without too much loss in visual quality. These percentages fluctuate depending on the
maximumScreenSpaceError
that is applied, and is more favorable for lower values.The settings can be tweaked by changing
tileset.usesDynamicScreenSpaceError
,tileset.dynamicScreenSpaceErrorDensity
, andtileset.dynamicScreenSpaceErrorFactor
.