Cache pickposition #5169

ateshuseyin · 2017-03-31T09:26:25Z

…ium.git into cache-pickposition

hpinkos · 2017-03-31T14:08:03Z

Thanks @ateshuseyin! It looks like you have a jsHint error:

Specs/Scene/SceneSpec.js
line 852  col 68  Unnecessary semicolon.

You can run jsHint locally with npm run jsHint

@lilleyse do you want to review this?

lilleyse · 2017-03-31T14:18:51Z

Yes, I should be able to review today.

lilleyse

Thanks @ateshuseyin for adding this ability. Just these comments.

lilleyse · 2017-03-31T14:44:43Z

Source/Scene/Scene.js

@@ -3043,6 +3046,21 @@ define([
     * @exception {DeveloperError} Picking from the depth buffer is not supported. Check pickPositionSupported.
     */
    Scene.prototype.pickPosition = function(windowPosition, result) {
+


The code added here belongs in pickPositionWorldCoordinates. The test may need to be adjusted for that.

lilleyse · 2017-03-31T14:45:50Z

Source/Scene/Scene.js

@@ -2616,6 +2616,7 @@ define([
     * @private
     */
    Scene.prototype.render = function(time) {
+        pickPositionCache = {}; //clean cache


Place this in function render instead.

lilleyse · 2017-03-31T14:52:55Z

Source/Scene/Scene.js

+                return cached.clone();
+            }
+
+            return cached;


This area can be simplified to return Cartesian2.clone(cached, result). If result is undefined it will create a new Cartesian2. If cached is undefined it will return undefined.

lilleyse · 2017-03-31T14:54:53Z

Source/Scene/Scene.js

@@ -3043,6 +3046,21 @@ define([
     * @exception {DeveloperError} Picking from the depth buffer is not supported. Check pickPositionSupported.
     */
    Scene.prototype.pickPosition = function(windowPosition, result) {
+
+        if(defined(windowPosition) && pickPositionCache.hasOwnProperty(windowPosition.toString())){
+            var cached = pickPositionCache[windowPosition.toString()];


windowPosition.toString() can be saved to its own variable so it is not computed multiple times.

You may want to add a comment somewhere that the cached result could be undefined. This way it is clear to whoever is reading that hasOwnProperty is used instead of just defined.

lilleyse · 2017-03-31T15:04:22Z

Source/Scene/Scene.js

@@ -3053,6 +3071,9 @@ define([
            var cart = projection.unproject(result, scratchPickPositionCartographic);
            ellipsoid.cartographicToCartesian(cart, result);
        }
+
+        pickPositionCache[windowPosition.toString()] = defined(result) ? result.clone() : result;


Same comment here about using Cartesian2.clone(result)

lilleyse · 2017-03-31T15:09:43Z

Specs/Scene/SceneSpec.js

+            expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);
+
+            s.pickPosition(windowPosition);
+            expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);


It's fine to just do just one expect(s.pickPositionWorldCoordinates) check at the end.

Second expect is necessary I think. First pickPosition is calculated and call count increment but at second result is returned from cache

One check should be fine because if the result is 2 then all the previous steps have used the cache correctly. But I'm also ok with keeping it as-is.

lilleyse · 2017-03-31T15:09:54Z

Specs/Scene/SceneSpec.js

+
+        var canvas = s.canvas;
+        var windowPosition = new Cartesian2(canvas.clientWidth / 2, canvas.clientHeight / 2);
+        spyOn(s, 'pickPositionWorldCoordinates').and.callThrough();;


Extra ; as @hpinkos mentioned.

lilleyse · 2017-03-31T15:10:22Z

Specs/Scene/SceneSpec.js

+        //     position = s.pickPosition(windowPosition);
+
+        //     expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(1);
+        // });


Remove commented code.

lilleyse · 2017-03-31T15:10:41Z

Source/Scene/Scene.js

@@ -3024,7 +3025,9 @@ define([

    var scratchPickPositionCartographic = new Cartographic();

-    /**
+    var pickPositionCache={};


Style: var pickPositionCache = {};

In the case of multiple scenes, this may be better as a private variable of Scene.

ateshuseyin · 2017-04-02T12:27:00Z

fixed belong to review.

lilleyse · 2017-04-03T14:32:54Z

Source/Scene/Scene.js

@@ -2523,6 +2525,8 @@ define([
    var scratchEyeTranslation = new Cartesian3();

    function render(scene, time) {
+        scene._pickPositionCache = {}; //clean <cache></cache>


Fix comment.

lilleyse · 2017-04-03T14:33:01Z

Source/Scene/Scene.js

@@ -2616,6 +2620,7 @@ define([
     * @private
     */
    Scene.prototype.render = function(time) {
+


Remove empty line.

lilleyse · 2017-04-03T14:33:25Z

Source/Scene/Scene.js

@@ -3024,7 +3029,7 @@ define([

    var scratchPickPositionCartographic = new Cartographic();

-    /**
+     /**


Undo this change, the old formatting is consistent with the other comments.

lilleyse · 2017-04-03T14:35:58Z

Source/Scene/Scene.js

@@ -3043,6 +3048,16 @@ define([
     * @exception {DeveloperError} Picking from the depth buffer is not supported. Check pickPositionSupported.
     */
    Scene.prototype.pickPosition = function(windowPosition, result) {
+        var cacheKey;


As mentioned before, the caching belongs in pickPositionWorldCoordinates. For example, both Camera and ScreenSpaceCameraController call it instead of pickPosition.

lilleyse · 2017-04-03T14:36:54Z

Source/Scene/Scene.js

+        var cacheKey;
+
+        if(defined(windowPosition)){
+            cacheKey = windowPosition.x + '-' + windowPosition.y;


Why not use toString like before?

lilleyse · 2017-04-03T14:38:15Z

Source/Scene/Scene.js

+
+        if(defined(windowPosition) && this._pickPositionCache.hasOwnProperty(cacheKey)){
+            return Cartesian2.clone(this._pickPositionCache[cacheKey], result);
+        }


When this code is moved to pickPositionWorldCoordinates the defined(windowPosition) checks here and above won't be needed.

lilleyse · 2017-04-03T14:38:45Z

Source/Scene/Scene.js

+            cacheKey = windowPosition.x + '-' + windowPosition.y;
+        }
+
+        if(defined(windowPosition) && this._pickPositionCache.hasOwnProperty(cacheKey)){


Style, add space: if (defined(windowPosition)

lilleyse · 2017-04-03T14:41:47Z

Specs/Scene/SceneSpec.js

+            expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);
+
+            s.pickPosition(windowPosition);
+            expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);


One check should be fine because if the result is 2 then all the previous steps have used the cache correctly. But I'm also ok with keeping it as-is.

lilleyse · 2017-04-03T14:44:17Z

Specs/Scene/SceneSpec.js

+            s.pickPosition(windowPosition);
+            expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);
+        });
+


Remove empty line.

First pickPosition calculates and returns undefined. The second one returns undefined from cache. Third calculates a valid Cartesian3 then forth one returns valid value from cache. So both calls are needed to test correctly. Because the flow of undefined and valid results are seperate.

pjcozzi · 2017-04-03T17:25:05Z

Thanks for this contribution, @ateshuseyin!

@lilleyse please ping me when you think this is ready, I'd like to take a quick look before merging into master.

ateshuseyin · 2017-04-03T21:14:39Z

Thanks cesium. It is great work.

I think all done.

lilleyse · 2017-04-06T13:36:35Z

Source/Scene/Scene.js

@@ -2954,6 +2958,12 @@ define([
        }
        //>>includeEnd('debug');

+        var cacheKey;


cacheKey needs to be set.

…ium.git into cache-pickposition # Conflicts: # Source/Scene/Scene.js

ateshuseyin · 2017-04-10T10:00:44Z

@lilleyse I think this is last commit :)

lilleyse · 2017-04-10T13:21:15Z

@pjcozzi this is ready for you to look at.

pjcozzi · 2017-04-12T21:58:26Z

Source/Scene/Scene.js

@@ -2548,6 +2550,8 @@ define([
    var scratchEyeTranslation = new Cartesian3();

    function render(scene, time) {
+        scene._pickPositionCache = {}; //clean cache


Can we avoid doing this allocation per frame? We are really careful about not doing allocation per vertex, per command, per primitive, etc., but we even try to avoid per frame allocations.

Instead, perhaps set a flag like _pickPositionCacheDirty to true. Then in pickPositionWorldCoordinates, clear the cache and set dirty to false if it was true.

pjcozzi · 2017-04-12T21:59:44Z

Just that comment. Thanks again @ateshuseyin, nice job with the tests!

ateshuseyin · 2017-04-14T20:51:54Z

@pjcozzi done

lilleyse · 2017-04-18T14:05:03Z

This looks ready to me, @pjcozzi do you want to merge?

pjcozzi · 2017-04-18T17:03:45Z

Looks good, but are we sure about all the changes to the .idea/* files that came in with 4354961?

lilleyse · 2017-04-18T17:09:16Z

I think those are related to the recent Webstorm update, which haven't been incorporated into 3d-tiles yet. I'll merge master into 3d-tiles first to be sure.

lilleyse · 2017-04-18T17:25:18Z

They aren't in master, so I just reverted those changes.

pjcozzi · 2017-04-18T17:27:20Z

Thanks @ateshuseyin @lilleyse!

Hüseyin ATEŞ added 3 commits March 30, 2017 14:55

Scene.pickPosition cache implemented

c19e304

Scene.pickPosition cache fixed for undefined results

5a5e386

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/ces…

f1eb982

…ium.git into cache-pickposition

lilleyse reviewed Mar 31, 2017

View reviewed changes

Hüseyin ATEŞ and others added 2 commits March 31, 2017 18:50

code fix belong to review

7dbe7af

fixes according to code review and jshint

ca9943e

lilleyse reviewed Apr 3, 2017

View reviewed changes

pickPosition cache code fix

6221b4d

lilleyse reviewed Apr 6, 2017

View reviewed changes

Hüseyin ATEŞ added 3 commits April 10, 2017 12:29

pickPosition cacheKey initiation

d6baa02

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/ces…

0286fa4

…ium.git into cache-pickposition # Conflicts: # Source/Scene/Scene.js

merge master

e1f1fd1

Remove originaltransformWindowToDrawingBuffer

3f7f48d

pjcozzi reviewed Apr 12, 2017

View reviewed changes

ateshuseyin added 2 commits April 14, 2017 23:34

pickPositionCacheDirty added

b7037d0

merge with master

4354961

Tweak

8003650

Merge branch 'master' into cache-pickposition

5ea9b54

Revert webstorm settings

e92cebd

pjcozzi merged commit 11aa2bc into CesiumGS:master Apr 18, 2017

ateshuseyin deleted the cache-pickposition branch May 20, 2017 20:00

Cache pickposition #5169

Cache pickposition #5169

Conversation

ateshuseyin commented Mar 31, 2017

hpinkos commented Mar 31, 2017

lilleyse commented Mar 31, 2017

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Mar 31, 2017 • edited Loading

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

ateshuseyin commented Apr 2, 2017

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

pjcozzi commented Apr 3, 2017

ateshuseyin commented Apr 3, 2017

Choose a reason for hiding this comment

ateshuseyin commented Apr 10, 2017

lilleyse commented Apr 10, 2017

Choose a reason for hiding this comment

pjcozzi commented Apr 12, 2017

ateshuseyin commented Apr 14, 2017

lilleyse commented Apr 18, 2017

pjcozzi commented Apr 18, 2017

lilleyse commented Apr 18, 2017

lilleyse commented Apr 18, 2017

pjcozzi commented Apr 18, 2017

lilleyse Mar 31, 2017 •

edited

Loading