Request render optin mode #6065

ggetz · 2017-12-19T19:28:40Z

Created opt-in requestRenderMode which will only render the scene when needed. Changes it watches for are currently minimal, but can be triggered as needed by calling requestRender().

Added events to TaskProcessor on a successful completed web worker task and RequestScheduler on a successful request that the scene can watch for. (I'm thinking these might be better off private?)
Camera changes and window resizing will cause a render
If the simulation time change exceeds scene.maximumRenderTimeChange, defaults to 0.5 and can be set to undefined to never change based on simulation time.
new public function scene.requestRender for forcing render in this mode

debugShowFrames per second might be misleading, the updates are still happening at a higher frame rate than what's being rendered, and the fps value is not updated until after a new render. Should I update the display to reflect this or leave it as is?

cesium-concierge · 2017-12-19T19:28:41Z

@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.

🌍 🌎 🌏

pjcozzi · 2017-12-21T23:38:20Z

Added events to TaskProcessor on a successful completed web worker task and RequestScheduler on a successful request that the scene can watch for. (I'm thinking these might be better off private?)

In general, if we don't have a concrete use case for making something public, keep it private. Less API surface area is less work and less maintenance.

pjcozzi · 2017-12-21T23:40:58Z

debugShowFrames per second might be misleading, the updates are still happening at a higher frame rate than what's being rendered, and the fps value is not updated until after a new render. Should I update the display to reflect this or leave it as is?

I don't 100% follow. Do you mean that the fps might be basically 0 when nothing is happening or it, for example, 3 when the scene is only rendered once in awhile due to tile downloads, etc.?

If so, perhaps report:

n fps when rendering full speed
n/a fps when not rendering
n fps (throttled) when rendering on demand

pjcozzi · 2017-12-21T23:41:24Z

Part of #1865

pjcozzi · 2017-12-21T23:43:03Z

@wallw-bits @chris-cooper @willemvdg @kring testing and code reviews are appreciated here, thanks!

pjcozzi

Looks like a nice start to CPU usage improvements!

Didn't look at the tests.

pjcozzi · 2017-12-21T23:48:49Z

Apps/Sandcastle/gallery/Scene Rendering Performance.html

@@ -0,0 +1,123 @@
+<!DOCTYPE html>


Is there any screenshot or graphic that could be used for this Sandcastle example?

pjcozzi · 2017-12-21T23:51:17Z

CHANGES.md

@@ -3,6 +3,9 @@ Change Log

 ### 1.41 - 2017-01-02

+* Added optional scene request render mode to reduce CPU usage


Given the holiday, I don't think this will make the 1.41 release.

pjcozzi · 2017-12-21T23:54:57Z

Source/Scene/Scene.js

+     * @see Scene#requestRenderMode
+     */
+    Scene.prototype.requestRender = function() {
+        if (!this._isRendering) {


Why check, just set it to true.

pjcozzi · 2017-12-21T23:55:58Z

Source/Scene/Scene.js

@@ -709,6 +719,37 @@ define([
        this._cameraVR = undefined;
        this._aspectRatioVR = undefined;

+        /**
+         * When <code>true</code>, rendering a frame will only occur when needed as determined by changes within the scene.


Please provide more detail here just like you did in the first comment in this pull request. In particular, make sure that the reader understands that they will need to explicitly call requestRender under many cases.

pjcozzi · 2017-12-21T23:58:14Z

Source/Scene/Scene.js

+        this._isRendering = true;
+
+        /**
+         * If {@link requestRenderMode} is <code>true</code>, this value defines the maximum change in


Provide more context and implications - this will impact sun lighting, water animation, and the choppiness of CZML animation, for example.

pjcozzi · 2017-12-21T23:59:32Z

Source/Scene/Scene.js

+         * @default false
+         */
+        this.requestRenderMode = defaultValue(options.requestRenderMode, false);
+        this._isRendering = true;


Is _isRendering the best name? Should this be _requestRender, _renderDirty, etc.?

ggetz · 2017-12-22T17:15:30Z

Fixed test failures and addressed comments from @pjcozzi

lilleyse

This seems really promising so far. I ran a bunch of Sandcastle examples and everything looked correct, if the camera doesn't move or things aren't loading the animations ticks every 0.5 seconds. I ran the 3D Tiles picking demo and forced a scene render for ever mouse move event which worked well.

I didn't go crazy with the benchmarking, but there is a clear difference between the optin mode on vs. off. Looks good.

lilleyse · 2018-01-02T18:26:53Z

Apps/Sandcastle/gallery/Scene Rendering Performance.html

+function startup(Cesium) {
+    'use strict';
+//Sandcastle_Begin
+var viewer = new Cesium.Viewer('cesiumContainer', {


It would be helpful to see a text box that says when the frame is rendered. That way someone using the demo will notice that rendering is enabled when the camera moves or when they click the request render button, and otherwise only renders once every 0.5 seconds.

I added a field with a timestamp of when the frame was last rendered.

lilleyse · 2018-01-02T18:34:36Z

Source/Core/RequestScheduler.js

@@ -154,6 +165,7 @@ define([
            --numberOfActiveRequestsByServer[request.serverKey];
            request.state = RequestState.RECEIVED;
            request.deferred.resolve(results);
+            requestLoadedEvent.raiseEvent();


To be consistent with TaskProcessor, should the raiseEvent should go before the resolve?

There is one other area where the raiseEvent should be triggered.

if (isDataUri(request.url) || isBlobUri(request.url)) { request.state = RequestState.RECEIVED; return request.requestFunction(); }

This is a quick path for data uris and blob uris.

Also add a test for this case.

lilleyse · 2018-01-02T18:45:43Z

Source/Scene/PerformanceDisplay.js

        var time = getTimestamp();

        this._fpsFrameCount++;
        var fpsElapsedTime = time - this._lastFpsSampleTime;
        if (fpsElapsedTime > 1000) {
-            var fps = this._fpsFrameCount * 1000 / fpsElapsedTime | 0;
+            var fps = 'N/A';
+            if (renderedThisFrame) {


The fps counters in the Cesium Inspector and 3D Tiles Inspector widgets are showing up as NA, probably not taking into account renderedThisFrame.

.

lilleyse · 2018-01-02T18:48:42Z

Source/Scene/Scene.js

@@ -210,6 +218,8 @@ define([
     * @param {Number} [options.terrainExaggeration=1.0] A scalar used to exaggerate the terrain. Note that terrain exaggeration will not modify any other primitive as they are positioned relative to the ellipsoid.
     * @param {Boolean} [options.shadows=false] Determines if shadows are cast by the sun.
     * @param {MapMode2D} [options.mapMode2D=MapMode2D.INFINITE_SCROLL] Determines if the 2D map is rotatable or can be scrolled infinitely in the horizontal direction.
+     * @param {Boolean} [options.requestRenderMode=false] If true, rendering a frame will only occur when needed as determined by changes within the scene. Enabling improves performance of the application.


It might be worth mentioning the possible caveats in this description as well.

lilleyse · 2018-01-02T18:55:16Z

Source/Scene/Scene.js

+         * When <code>true</code>, rendering a frame will only occur when needed as determined by changes within the scene.
+         * Enabling improves performance of the application, but requires using {@link requestRender}
+         * to render a new frame explicitly in this mode. This will be necessary in many cases after making updates
+         * to the scene.


This doc needs to be more direct: This will be necessary in many cases after making updates to the scene. should directly say that the user needs to call requestRender if they make changes to the scene.

lilleyse · 2018-01-02T19:17:15Z

Source/Scene/Scene.js

+        }
+        if (defined(this._removeTaskProcessorListenerCallback)) {
+            this._removeTaskProcessorListenerCallback();
+        }


These should be defined always.

lilleyse · 2018-01-02T19:21:17Z

Source/Scene/Scene.js

@@ -3563,5 +3653,15 @@ define([
        return SceneTransforms.wgs84ToWindowCoordinates(this, position, result);
    };

+    /**
+     * Requests a new rendered frame when {@link requestRenderMode} is set to <code>true</code>.


Same note about the link not working.

lilleyse · 2018-01-02T19:23:18Z

Source/Scene/Scene.js

+     */
+    Scene.prototype.requestRender = function() {
+        this._renderRequested = true;
+    };


For organizational purposes this and cartesianToCanvasCoordinates should come before isDestroyed, and preferably closer to similar areas of the code.

lilleyse · 2018-01-02T19:27:01Z

Specs/Core/TaskProcessorSpec.js

+        };
+        var eventRaised = false;
+        var removeListenerCallback = TaskProcessor.taskCompletedEvent.addEventListener(function () {
+            console.log('2');


Remove this.

lilleyse · 2018-01-02T19:30:24Z

Source/Scene/Scene.js

+
+        var shouldRender = !this.requestRenderMode || this._renderRequested || cameraChanged || (this.mode === SceneMode.MORPHING);
+
+        var now = JulianDate.clone(time);


Use a scratch variable instead, and clone into _lastRenderTime inside if (shouldRender).

ggetz · 2018-01-03T15:37:25Z

Thanks @lilleyse! Updated according to your comments.

Also,

if the camera doesn't move or things aren't loading the animations ticks every 0.5 seconds

The maxiumRenderTimeChange controls this update rate, so yo can do it less frequently (or not at all) if your scene has no animations, or more often if you need smoother animations.

lilleyse · 2018-01-03T15:45:53Z

Cool I'll look at the recent updates.

The maxiumRenderTimeChange controls this update rate, so yo can do it less frequently (or not at all) if your scene has no animations, or more often if you need smoother animations.

Yup, I was just stating that things were operating correctly.

lilleyse · 2018-01-03T15:54:18Z

There are a couple eslint errors.

ggetz · 2018-01-03T17:59:53Z

@lilleyse Sorry, they're all cleaned up!

lilleyse · 2018-01-03T15:56:15Z

Source/Scene/PerformanceDisplay.js

@@ -88,15 +88,18 @@ define([
    /**
     * Update the display.  This function should only be called once per frame, because
     * each call records a frame in the internal buffer and redraws the display.
+     *
+     * @param {Boolean} [renderedThisFrame] If provided, the FPS count will only update and display if true.


[renderedThisFrame=true]

lilleyse · 2018-01-03T19:03:42Z

Source/Scene/Scene.js

+     * @param {Boolean} [options.requestRenderMode=false] If true, rendering a frame will only occur when needed as
+     *  determined by changes within the scene. Enabling improves performance of the application, but requires using
+     *  {@link Scene#requestRender} to render a new frame explicitly in this mode. This will be necessary in many cases
+     *  after making changes to the scene in other parts of the API.


I think we typically keep parameter doc on a single line even it's long.

lilleyse · 2018-01-03T19:07:02Z

Source/Scene/Scene.js

            shouldRender = shouldRender || difference >= this.maximumRenderTimeChange;
        }

        try {
            if (shouldRender) {
-                this._lastRenderTime = now;
+                this._lastRenderTime = JulianDate.clone(time, scratchTime);;


Actually now that I see the updated changes just remove the scratch and call JulianData.clone(time, this._lastRenderTime).

lilleyse · 2018-01-03T19:14:05Z

It looks like a couple review comments from the latest review got hidden, make sure to expand all.

lilleyse · 2018-01-03T19:17:06Z

I wonder if there's more we can do to improve globe/imagery loading. For example, a simple Sandcastle like:

var viewer = new Cesium.Viewer('cesiumContainer', {
    requestRenderMode : true,
    maximumRenderTimeChange : 100
});

will not show the globe. I'm guessing globe's render and update are tightly coupled, and am curious if there's anything we can do about that.

lilleyse · 2018-01-03T19:18:49Z

Missed this on the first go, but Viewer and CesiumWidget should have tests, including a test for resize if possible,

ggetz · 2018-01-03T19:32:49Z

I wonder if there's more we can do to improve globe/imagery loading. For example, a simple Sandcastle like ... will not show the globe. I'm guessing globe's render and update are tightly coupled, and am curious if there's anything we can do about that.

This is exactly what my next PR addresses. The tile loading for the globe was tied to the render loop, so nothing was getting loaded without rendering a new frame. I separated the update/render logic there, so the globe always loads in completely without time changes triggering the render.

lilleyse · 2018-01-03T19:56:39Z

This is exactly what my next PR addresses. The tile loading for the globe was tied to the render loop, so nothing was getting loaded without rendering a new frame. I separated the update/render logic there, so the globe always loads in completely without time changes triggering the render.

Ok, awesome!

ggetz · 2018-01-03T20:34:08Z

Addressed your additional feedback @lilleyse, only #6065 (comment) is outstanding.

ggetz · 2018-01-04T17:15:57Z

@lilleyse Updated the events (and changed the structure of the render function to reflect that. The update function is empty, but that is where the majority of the refactored code will go for the globe updates talked about above.). I added preUpdate and postUpdate events, and check for rendering after these events are triggered. preRender and postRender are only triggered when the scene renders a new frame. Updated docs and CHANGES.md to reflect the changes.

lilleyse · 2018-01-04T17:25:20Z

Ok, I will look over the new changes a bit later today.

lilleyse · 2018-01-05T00:16:16Z

CHANGES.md

-* Added optional scene request render mode to reduce CPU usage
-    * `scene.requestRenderMode` enables a mode which will only request new render frames on changes to the scene, or when the simulation time change exceeds `scene.maximumRenderTimeChange`.
+* Breaking changes
+    * `scene.preRender` and `scene.postRender` events are called immediately before and after scene rendering only if the scene renders a frame. See `scene.requestRenderMode`. Use `scene.preUpdate` and `scene.postUpdate` for task that require regular updates.


I actually don't think this needs to be a breaking change. Though the connection between render and update events still needs to be clear. Maybe below write:

Added scene.preUpdate and scene.postUpdate events that are raised before and after the scene updates respectively. The scene is always updated before executing a potential render. Continue to listen to scene.preRender and scene.postRender for when the scene renders a frame.

lilleyse · 2018-01-05T00:16:31Z

CHANGES.md

-* Added optional scene request render mode to reduce CPU usage
-    * `scene.requestRenderMode` enables a mode which will only request new render frames on changes to the scene, or when the simulation time change exceeds `scene.maximumRenderTimeChange`.
+* Breaking changes
+    * `scene.preRender` and `scene.postRender` events are called immediately before and after scene rendering only if the scene renders a frame. See `scene.requestRenderMode`. Use `scene.preUpdate` and `scene.postUpdate` for task that require regular updates.


Capitalize Scene for all these.

lilleyse · 2018-01-05T00:17:42Z

Source/Scene/FrameRateMonitor.js

@@ -98,7 +98,7 @@ define([
        this._pauseCount = 0;

        var that = this;
-        this._preRenderRemoveListener = this._scene.preRender.addEventListener(function(scene, time) {


There are still a lot of other files and Sandcastle demos that reference preRender and postRender, do those need to use the update listeners instead too?

Usage of postRender didn't need to be changed in the examples, but I modified the preRender events where necessary.

lilleyse · 2018-01-05T00:19:48Z

Source/Scene/Scene.js

+         * @see scene#postUpdate
+         * @see scene#preRender
+         * @see scene#postRender
+         * @see scene#render


Capitalize Scene for these and below.

lilleyse · 2018-01-05T00:20:15Z

Source/Scene/Scene.js

+         * @see scene#postUpdate
+         * @see scene#preRender
+         * @see scene#postRender
+         * @see scene#render


Remove the link to scene#render, it is a private function.

Same below.

lilleyse · 2018-01-05T00:21:48Z

Source/Scene/Scene.js

@@ -2787,7 +2840,7 @@ define([
        }
    }

-    function callAfterRenderFunctions(frameState) {
+    function callAfterRenderCycleFunctions(frameState) {


Why the name change?

I wanted to make it clear that it was called every call to scene.render, whether a new frame is rendered or not, but it's not necessary in retrospect.

lilleyse · 2018-01-05T00:23:38Z

Source/Scene/Scene.js

+        try {
+            functionToExecute(scene, time);
+        } catch (error) {
+            scene._renderError.raiseEvent(scene, error);


Both update and render use renderError. Do we need an updateError?

The only place it's used in Cesium is that the viewer listens to it, and stops the render loop and displays it, which we'd still want. In case any app is also listening for this event, errors occurring during update would no longer be caught, which might be an issue.

Both are occurring during the scene.render function, so I don't think the naming is an issue, but if we wanted we could rename and deprecate scene.renderError

Probably okay to leave as is for now. Maybe we can reevaluate for the upcoming PR.

lilleyse · 2018-01-05T00:28:21Z

Specs/Scene/SceneSpec.js

+        s.destroyForSpecs();
+    });
+
+    it('always raises preUpdate event after updating', function() {


This is a duplicate of the other test. Should this be postUpdate?

lilleyse · 2018-01-05T18:46:01Z

The recent changes look good.

Would anyone else like to review? The PR is being merged into cpu-usage so there will be an opportunity to review everything once all the incremental PRs are done.

lilleyse · 2018-01-08T15:23:29Z

Merging into cpu-usage.

ggetz added 5 commits December 18, 2017 14:27

Scene render loop updates with opt in requestRender mode

38d7303

Merge branch 'master' into requestRender-optin-mode

97f226f

Sandcastle updates and specs

ddbd5e3

Update CHANGES.md

f215d5c

Merge branch 'master' into requestRender-optin-mode

06270d5

ggetz requested a review from mramato December 19, 2017 19:28

ggetz added 5 commits December 20, 2017 11:55

Fix eslint errors

1789480

Eslint

b2b08b8

Fixes

9c11b92

Include scene morphing and update tests

94165ba

Callback fix

0527041

pjcozzi mentioned this pull request Dec 21, 2017

Cesium CPU usage #1865

Closed

pjcozzi requested a review from lilleyse December 21, 2017 23:43

pjcozzi assigned lilleyse and mramato Dec 21, 2017

Fix typo in CHANGES.md

93a40af

pjcozzi requested changes Dec 22, 2017

View reviewed changes

ggetz added 5 commits December 22, 2017 10:16

Update CHANGES.md, docs

87a10e8

Code fixes

1f20508

Update performance display

ee37455

Cleanup scene specs

8eace56

Sandcastle example thumbnail

9659b2c

lilleyse reviewed Jan 2, 2018

View reviewed changes

Cleanup, update docs, tests, and displays for Sandcastle

3a7e9f2

Eslint errors fix

72cacdb

lilleyse reviewed Jan 3, 2018

View reviewed changes

CesiumWidget and Viewer requestRender specs

a99bc64

CesiumGS deleted a comment from biomassives Jan 3, 2018

Rendering/Update events

d698876

lilleyse reviewed Jan 5, 2018

View reviewed changes

Update Sandacastle render event usgae, tweak CHANGES.md

14b7cb4

lilleyse merged commit f69edc1 into cpu-usage Jan 8, 2018

lilleyse deleted the requestRender-optin-mode branch January 8, 2018 15:23

This was referenced Jan 10, 2018

Globe imagery and terrain loading trigger renders in requestRenderMode #6107

Merged

CPU Usage #6115

Merged

		@@ -3,6 +3,9 @@ Change Log

		### 1.41 - 2017-01-02

		* Added optional scene request render mode to reduce CPU usage


		var shouldRender = !this.requestRenderMode \|\| this._renderRequested \|\| cameraChanged \|\| (this.mode === SceneMode.MORPHING);

		var now = JulianDate.clone(time);

Request render optin mode #6065

Request render optin mode #6065

Conversation

ggetz commented Dec 19, 2017

cesium-concierge commented Dec 19, 2017

pjcozzi commented Dec 21, 2017

pjcozzi commented Dec 21, 2017

pjcozzi commented Dec 21, 2017

pjcozzi commented Dec 21, 2017

pjcozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Dec 22, 2017

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Jan 3, 2018

lilleyse commented Jan 3, 2018

lilleyse commented Jan 3, 2018 • edited Loading

ggetz commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jan 3, 2018

lilleyse commented Jan 3, 2018

lilleyse commented Jan 3, 2018

ggetz commented Jan 3, 2018

lilleyse commented Jan 3, 2018

ggetz commented Jan 3, 2018

ggetz commented Jan 4, 2018

lilleyse commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jan 5, 2018

lilleyse commented Jan 8, 2018

lilleyse commented Jan 3, 2018 •

edited

Loading