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

CPU Usage #6115

Merged
merged 56 commits into from
Jan 29, 2018
Merged

CPU Usage #6115

merged 56 commits into from
Jan 29, 2018

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jan 12, 2018

Changes from PRs #6065 and #6107
Opt in fix for #1865

Created opt-in requestRenderMode which will only render the scene when needed. Renders can be triggered as needed by calling requestRender(), for example if picking a feature should change it's color, or you make changes to the scene via the API.

We render each time the simulation time change exceeds scene.maximumRenderTimeChange, which defaults to 0.5 seconds. It can be set to undefined or Infinity to never change based on simulation time.

Besides simulation time changes and calls to requestRender, always render when:

  • The camera changes
  • Web worker returns, successfully or with an error
  • RequestScheduler fulfills a request, successfully or not
  • A FrameState.afterRender function was called (this is used thorough the code base when changes are made after a render to keep the current state. An example would be when a new LOD of a tileset is shown or when a model is ready)
  • Globe tiles are loaded
  • ImageryLayers are added, removes, shown, hidden, or moved.
  • The scene morphs

In addition, I refactored some of the update/render logic in Globe, QuadtreePrimitive, and GlobeSurfaceTileProvider so that loading new tiles will continue to prompt render requests and finish processing the load queue. When the scene has requestRenderMode enabled and there is a large or undefined maximumRenderTimeChange, the globe will still load in completely, and changing terrain providers or imagery, and loading in higher level tiles when the camera changes will automatically trigger a render.

@cesium-concierge
Copy link

Signed CLA is on file.

@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@mramato
Copy link
Contributor

mramato commented Jan 25, 2018

Double clicking on an Entity does not cause a render, I would expect this to happen because it is part of the Viewer widget (and camera interaction)

@mramato
Copy link
Contributor

mramato commented Jan 25, 2018

Same bug (probably same root cause) happens when you click on the camera icon on the InfoBox.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 25, 2018

Double clicking on an Entity does not cause a render

Hmm, that's weird. I would expect that to be caught with the camera change event since it's moving the camera and changing the transform

@hpinkos
Copy link
Contributor

hpinkos commented Jan 25, 2018

Everything else looks fine to me! Who else wants to review this before it's merged? I'll plan on merging this first thing Monday morning unless someone wants more time to review (assuming all bugs are fixed).

@ggetz
Copy link
Contributor Author

ggetz commented Jan 25, 2018

Hmm, that's weird. I would expect that to be caught with the camera change event since it's moving the camera and changing the transform

I would also expect the camera change to prompt a render, I'll take a look.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 25, 2018

I'm seeing a test failure

Scene/Scene scene morphing causes a new frame to be rendered in requestRenderMode
Expected JulianDate({ dayNumber: 2458144, secondsOfDay: 29660.149 }) not to equal JulianDate({ dayNumber: 2458144, secondsOfDay: 29660.149 }).
Error: Expected JulianDate({ dayNumber: 2458144, secondsOfDay: 29660.149 }) not to equal JulianDate({ dayNumber: 2458144, secondsOfDay: 29660.149 }).
    at stack (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1475:17)
    at buildExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1445:14)
    at Spec.expectationResultFactory (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:584:18)
    at Spec.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:332:34)
    at Expectation.addExpectationResult (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:528:21)
    at Expectation.toEqual (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1399:12)
    at http://localhost:8080/Specs/Scene/SceneSpec.js:1545:42
    at Object.<anonymous> (http://localhost:8080/Specs/spec-main.js:142:30)
    at attemptAsync (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
    at QueueRunner.run (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1746:26)

@ggetz
Copy link
Contributor Author

ggetz commented Jan 25, 2018

@hpinkos Cleaned up the examples, and made the morphing specs more reliable.

Setting trackedEntity now prompts a render. This resolves the two bugs @mramato brought up, as they were setting tracked entity, but the camera didn't update until the next frame.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 26, 2018

I think you missed this one

@ggetz add a comment about fixing #4753 to CHANGES

@mramato
Copy link
Contributor

mramato commented Jan 26, 2018

I think we should enable requestRenderMode in CesiumViewer.js, since all it does is expose the Viewer widget and therefore works out of the box with the latest round of changes!

@ggetz
Copy link
Contributor Author

ggetz commented Jan 26, 2018

@hpinkos Thanks, fixed!

Add requestRenderMode to the CeisumViewer, and both build and unbuilt are working as is. 👍

@ggetz
Copy link
Contributor Author

ggetz commented Jan 29, 2018

@hpinkos Good to merge?

@hpinkos
Copy link
Contributor

hpinkos commented Jan 29, 2018

Taking a final look at this now, thanks for the reminder @ggetz!

@hpinkos
Copy link
Contributor

hpinkos commented Jan 29, 2018

Great work on this @ggetz!

@hpinkos hpinkos merged commit eaf2779 into master Jan 29, 2018
@hpinkos hpinkos deleted the cpu-usage branch January 29, 2018 17:08
@ggetz
Copy link
Contributor Author

ggetz commented Jan 29, 2018

Thanks @hpinkos for all the reviews!

@thw0rted
Copy link
Contributor

Right now, when I toggle the show property for a DataSource in the dataSources collection, I have to explicitly request a render. Would it make sense to automatically observe the show property when adding a source to the collection, and re-render any time it changes? I guess you could actually watch at the Entity.show level, since ideally you'd re-render when any entity is shown or hidden, but I don't think there's a single easy event that fires any time an Entity that the Viewer "has" (in the defaultDataSource or somewhere in dataSources) is shown/hidden.

@mramato
Copy link
Contributor

mramato commented Feb 13, 2018

@thw0rted While at a high level, I agree with you, the problem of trying to handle all of these cases for end users is that it's tedious to maintain and hard to scale. I think one of the biggest scalability mistakes I made with the Entity API was trying to make everything automatically happen for the user. The end result is that browsers gobble up memory handling all of the subscriptions and notifications. So for request render mode, we made a conscious decision that the user would have to initiate the request. We may revisit this in the future (and god knows I would love to completely refactor the Entity API) but I don't expect that to happen in the near future.

That being said, while writing this reply I had an idea. You can actually just listen to the EntityCollection.collectionChanged event and call requestRender whenever it is fired. I think that will give you exactly what you want and may actually be simple and robust enough to warrant a PR into master. Do you have to give it a try and see how it works out for you?

Thanks.

@thw0rted
Copy link
Contributor

I had the same thought, having a sub to every entity's state would not scale very well, and I'm not sure how you even profile to figure out why it feels slow once it starts to bog down. I'll take a look at collectionChanged, see if it's a better fit for my model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants