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
20 changes: 19 additions & 1 deletion Source/Scene/Scene.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,8 @@ define([
this._cameraStartFired = false;
this._cameraMovedTime = undefined;

this._pickPositionCache = {};

/**
* Exceptions occurring in <code>render</code> are always caught in order to raise the
* <code>renderError</code> event. If this property is true, the error is rethrown
Expand Down Expand Up @@ -2523,6 +2525,8 @@ define([
var scratchEyeTranslation = new Cartesian3();

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

Choose a reason for hiding this comment

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

Fix comment.


if (!defined(time)) {
time = JulianDate.now();
}
Expand Down Expand Up @@ -2616,6 +2620,7 @@ define([
* @private
*/
Scene.prototype.render = function(time) {

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.

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

var scratchPickPositionCartographic = new Cartographic();

/**
/**
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 +3048,16 @@ define([
* @exception {DeveloperError} Picking from the depth buffer is not supported. Check pickPositionSupported.
*/
Scene.prototype.pickPosition = function(windowPosition, result) {
var cacheKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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)){
cacheKey = windowPosition.x + '-' + windowPosition.y;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use toString like before?

}

if(defined(windowPosition) && this._pickPositionCache.hasOwnProperty(cacheKey)){
Copy link
Contributor

Choose a reason for hiding this comment

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

Style, add space: if (defined(windowPosition)

return Cartesian2.clone(this._pickPositionCache[cacheKey], result);
}
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 +3068,9 @@ define([
var cart = projection.unproject(result, scratchPickPositionCartographic);
ellipsoid.cartographicToCartesian(cart, result);
}

this._pickPositionCache[cacheKey] = Cartesian2.clone(result);

return result;
};

Expand Down
37 changes: 37 additions & 0 deletions Specs/Scene/SceneSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,43 @@ 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();

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.

});

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