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

Skip LODs #5128

Merged
merged 78 commits into from
Apr 13, 2017
Merged

Skip LODs #5128

merged 78 commits into from
Apr 13, 2017

Conversation

austinEng
Copy link
Contributor

@austinEng austinEng commented Mar 20, 2017

This PR updates tile selection to skip levels of detail, often resulting in a 50% decrease in data downloaded.

Task list:

  • Add additional comments
  • Inherit original model backface culling properties
  • Additional testing
  • Flyover performance analysis
  • Analyze performance comparing old method with method with skipping disabled
  • Remove old code path and verify equivalent with new code path with skipping disabled
  • Translucent objects

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2017

Wow, this is in need of some optimizations! Wait for a reasonably big tileset to stop loading, then profile. There is a ton of overhead getting render states, cloning, and in the GC:

image

Compare the profile to the 3d-tiles branch.

This should be easy to fix with some better caches.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2017

I want to do more profiling, but #5128 (comment) needs to be addressed first. Please rise and repeat until you think it is reasonable compared to the 3d-tiles branch bottlenecks.

@austinEng
Copy link
Contributor Author

austinEng commented Apr 11, 2017

Wow, this is in need of some optimizations! Wait for a reasonably big tileset to stop loading, then profile. There is a ton of overhead getting render states, cloning, and in the GC:

??? @pjcozzi What are you testing with? These are my results with AGI:

skip-levels:
skip-levels

3d-tiles:
3d-tiles

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2017

Will send you the dataset offline.

@austinEng
Copy link
Contributor Author

@pjcozzi These are my results:

skip-levels:
skip-levels-2

3d-tiles:
3d-tiles-2

Not sure what's happening. Are you sure it was done loading? I get similar results like that if it's still loading.

@mramato
Copy link
Contributor

mramato commented Apr 11, 2017

Are you guys using the combined Cesium.js with debug code removed? Just a kindly reminder that all non-trivial performance testing should be done with the that build to avoid our developer error checks from skewing the numbers.

@lilleyse
Copy link
Contributor

lilleyse commented Apr 11, 2017

I'm seeing similar results to @pjcozzi with a release build
results

Like @pjcozzi this seems like a pretty simple fix. Back face commands could be saved in the command's derived commands rather than cloning every time. Also the stencil render state could be saved somewhere too, maybe also as part of a stencil derived command, or otherwise as just the render state. A tile's _selectionDepth wouldn't vary unless any of the skipping parameters are changed, right? Part of the problem is RenderState.fromCache is a pretty heavy operation, and the clone can be too.

EDIT: I don't think I'm right about _selectionDepth. That part could be trickier.

@austinEng
Copy link
Contributor Author

austinEng commented Apr 11, 2017

EDIT: I don't think I'm right about _selectionDepth. That part could be trickier.

We could just make the_selectionDepth property a setter and mark the commands as dirty. How are derived commands generated?

@lilleyse
Copy link
Contributor

It's not that well specified right now, but the main way is to add a property to the command's derivedCommands with the new command, like in Cesium3DTileBatchTable.prototype.getAddCommand.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2017

In the 3D Tiles roadmap, #3241, are any of the following items no longer relevant:

  • Performance: exploit temporal coherence in sortChildrenByDistanceToCamera
    - [ ] Sort by pixel size instead of distance? What about large bounding volumes with sparse contents?
    - [ ] Is selectTiles better implemented with a priority queue instead of explicit sort?

@lilleyse
Copy link
Contributor

@austinEng the separate CESIUM_3D_TILES pass is now in 3d-tiles. In order for per-tileset stencil clearing to work you will need to update ClearCommand to accept a pass. More info here: #5168 (comment). It may be easier to do this in a separate PR.

@austinEng
Copy link
Contributor Author

While it is more efficient to handle the root of an external tileset upfront like this, can it be removed and just handled with the rest of the traversal? My main worry is that it could be easy to forget which properties to be set on the child, especially when new properties are added.

@lilleyse Actually the change to do this here broke a bunch of tests. My guess is that this is because the original code just sets the visibilityPlaneMask to that of the other tile and ignores the external tileset's root bounding information. Is this the desired behavior or are our tests wrong?

@austinEng
Copy link
Contributor Author

@pjcozzi @lilleyse Updated to use derived commands to signficantly improve performance. My profiles are showing RenderState.fromCache and clone at 0.25% in the CPU profile for a loading tileset without camera movement. Moving the camera around a lot to cause aggressive loading bumps that up to just 3% so I think this is significantly better than the 25% numbers you were seeing.

Can you check your performance numbers?
Note: tests are broken due to this

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 12, 2017

Looks a lot better for my tests.

For my one tileset, the GC is high at ~3.2%, but it is similar in the 3d-tiles branch as well so it doesn't need to hold this PR up.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 12, 2017

So +1 from me to merge this when you guys are ready.

@lilleyse
Copy link
Contributor

clone and RenderState.fromCache are each between 1.0% and 2.0% for me now.

Double check that point clouds still work fine, since their commands do not go through the batch table. Looking at the code it should be fine since backFaceCommands.length would be 0.

@austinEng
Copy link
Contributor Author

@lilleyse Added the CESIUM_3D_TILE pass and resolved the issue with external tilesets

@lilleyse
Copy link
Contributor

Ran on a point cloud tileset for good measure.

Time to release it!!

@lilleyse lilleyse merged commit 548ee59 into CesiumGS:3d-tiles Apr 13, 2017
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