-
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
fix scene.updateHeight #12134
base: main
Are you sure you want to change the base?
fix scene.updateHeight #12134
Conversation
Thank you for the pull request, @bkuster! ✅ We can confirm we have a CLA on file for you. |
Hi @bkuster, thanks very much for the proposed fix!
Did you have a particular tileset in mind for testing? Google's tileset is certainly the most common use case here, but in my experience is not the worst case in terms of performance. |
Hi @ggetz here is a dataset you can use: but the dataset is hosted in Frankfurt, so hopefully fast enough in the US
|
@ggetz you can also add the following code to the sandcastle above to see the effect.
The Console log is called if getHeight is called more than once in a single Frame, in the main branch you can often see getHeight getting executed for the same cartograpic several times, if you move around in the scene. |
Thanks @jbo023 for the test case! @bkuster While I agree that improvement can be made in this area, I'm not sure this is a noticeable improvement. Using Google's 3D Tileset as well at the test case in this thread, I don't see a difference in FPS when navigating around the scene. Am I missing anything? @jbo023 do you notice an improvement in FPS? |
@ggetz yeah its a bit hard to see, adding clamped points to the scene will point out the issue a bit more. If you compare loading the following sandcastle you should see it. If i take a performance snapshot during loading, until 60 fps is reached, i see an improvement of aggregated time spend in executing 'Scene.getHeight` from around 10000ms to 5000ms
|
Description
There where some performance issues in
Scene.updateHeight
which made working with large (mesh surfaces in general) 3D Tiles difficult to very clunky.Proposed fixes
Cesium3DTileset.js
, update height callbacks where wrapped in an object, with aninvoked
property. This property was alwaysfalse
. In the PR, we set it to true once an update has been added to the frame state and set it back to false on flush.Scene.updateHeight
a callback can be called more then once for the same frame number. We now ensure it is only called once.Scene.updateHeight
a callback which was removed would still be called with a new height. When loading a mesh surface, multiple callbacks are added, one for each initialized frame. For each new one added, the old one is removed, but still called once. We now only call callbacks which where not removed.Testing plan
Add a mesh surface with
enableCollision
and watch your performance drop.Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change