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

Cache pickposition #5169

Merged
merged 15 commits into from
Apr 18, 2017
Merged
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ Change Log
* Fixed issues with imagerySplitPosition and the international date line in 2D mode. [#5151](https://github.com/AnalyticalGraphicsInc/cesium/pull/5151)
* Fixed an issue with `TileBoundingBox` that caused the terrain to disappear in certain places [4032](https://github.com/AnalyticalGraphicsInc/cesium/issues/4032)
* `QuadtreePrimitive` now uses `frameState.afterRender` to fire `tileLoadProgressEvent` [#3450](https://github.com/AnalyticalGraphicsInc/cesium/issues/3450)
* `Scene.pickPosition` now caches results per frame to increase performance [#5117](https://github.com/AnalyticalGraphicsInc/cesium/issues/5117)

### 1.31 - 2017-03-01

Expand Down
23 changes: 22 additions & 1 deletion Source/Scene/Scene.js
Original file line number Diff line number Diff line change
Expand Up @@ -2616,6 +2616,7 @@ define([
* @private
*/
Scene.prototype.render = function(time) {
pickPositionCache = {}; //clean cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place this in function render instead.

try {
render(this, time);
} catch (error) {
Expand Down Expand Up @@ -3024,7 +3025,9 @@ define([

var scratchPickPositionCartographic = new Cartographic();

/**
var pickPositionCache={};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: var pickPositionCache = {};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Returns the cartesian position reconstructed from the depth buffer and window position.
* <p>
* The position reconstructed from the depth buffer in 2D may be slightly different from those
Expand All @@ -3043,6 +3046,21 @@ define([
* @exception {DeveloperError} Picking from the depth buffer is not supported. Check pickPositionSupported.
*/
Scene.prototype.pickPosition = function(windowPosition, result) {

Copy link
Contributor

@lilleyse lilleyse Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if(defined(windowPosition) && pickPositionCache.hasOwnProperty(windowPosition.toString())){
var cached = pickPositionCache[windowPosition.toString()];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (defined(cached) && defined(result)){
result.x = cached.x;
result.y = cached.y;
}

if (defined(cached)){
return cached.clone();
}

return cached;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


result = this.pickPositionWorldCoordinates(windowPosition, result);
if (defined(result) && this.mode !== SceneMode.SCENE3D) {
Cartesian3.fromElements(result.y, result.z, result.x, result);
Expand All @@ -3053,6 +3071,9 @@ define([
var cart = projection.unproject(result, scratchPickPositionCartographic);
ellipsoid.cartographicToCartesian(cart, result);
}

pickPositionCache[windowPosition.toString()] = defined(result) ? result.clone() : result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about using Cartesian2.clone(result)


return result;
};

Expand Down
71 changes: 71 additions & 0 deletions Specs/Scene/SceneSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,77 @@ defineSuite([
});
});

it('pickPosition caches results per frame',function(){
var s = createScene();
if (!s.pickPositionSupported) {
return;
}

var rectangle = Rectangle.fromDegrees(-100.0, 30.0, -90.0, 40.0);
s.camera.setView({ destination : rectangle });

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra ; as @hpinkos mentioned.


expect(s).toRenderAndCall(function() {
s.pickPosition(windowPosition);
expect(s.pickPositionWorldCoordinates).toHaveBeenCalled();

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

var rectanglePrimitive = createRectangle(rectangle);
rectanglePrimitive.appearance.material.uniforms.color = new Color(1.0, 0.0, 0.0, 1.0);

var primitives = s.primitives;
primitives.add(rectanglePrimitive);
});

expect(s).toRenderAndCall(function() {
s.pickPosition(windowPosition);
expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);

s.pickPosition(windowPosition);
expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.




// var s = createScene();

// if (!s.pickPositionSupported) {
// return;
// }

// var rectangle = Rectangle.fromDegrees(-100.0, 30.0, -90.0, 40.0);
// s.camera.setView({
// destination : rectangle
// });

// var rectanglePrimitive = createRectangle(rectangle);
// rectanglePrimitive.appearance.material.uniforms.color = new Color(1.0, 0.0, 0.0, 1.0);

// var primitives = s.primitives;
// primitives.add(rectanglePrimitive);

// spyOn(s, 'pickPositionWorldCoordinates');

// var canvas = s.canvas;
// var windowPosition = new Cartesian2(canvas.clientWidth / 2, canvas.clientHeight / 2);
// s.useDepthPicking = true;

// expect(s).toRenderAndCall(function() {
// var position = s.pickPosition(windowPosition);

// expect(s.pickPositionWorldCoordinates).toHaveBeenCalled();
// position = s.pickPosition(windowPosition);

// expect(s.pickPositionWorldCoordinates.calls.count()).toEqual(1);
// });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented code.

});

it('pickPosition throws without windowPosition', function() {
expect(function() {
scene.pickPosition();
Expand Down