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

Added feature to modify scene pick search size #5602

Merged
merged 13 commits into from
Jul 13, 2017
3 changes: 3 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu

## [Corporate CLA](Documentation/Contributors/CLAs/corporate-cla-agi-v1.0.txt)

* [Bentley Systems ](https://www.bentley.com/)
* [Jason Crow](https://github.com/jason-crow)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's already a section for Bentley Systems at line 84. Add yourself under Paul Connelly instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, updated


* [Analytical Graphics, Inc.](http://www.agi.com/)
* [Patrick Cozzi](https://github.com/pjcozzi)
* [Kristian Calhoun](https://github.com/kristiancalhoun)
Expand Down
12 changes: 7 additions & 5 deletions Source/Scene/Scene.js
Original file line number Diff line number Diff line change
Expand Up @@ -2769,9 +2769,6 @@ define([
}

// pick rectangle width and height, assumed odd
var rectangleWidth = 3.0;
var rectangleHeight = 3.0;
var scratchRectangle = new BoundingRectangle(0.0, 0.0, rectangleWidth, rectangleHeight);
var scratchColorZero = new Color(0.0, 0.0, 0.0, 0.0);
var scratchPosition = new Cartesian2();

Expand All @@ -2793,16 +2790,22 @@ define([
* }, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
*
* @param {Cartesian2} windowPosition Window coordinates to perform picking on.
* @param {Number} [optional] width of the pick rectangle
* @param {Number} [optional] height of the pick rectangle
Copy link
Contributor

Choose a reason for hiding this comment

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

For optional arguments the name of the argument should be within the brackets, with default values shown, like:

@param {Number} [width=3] Width of the pick rectangle.
@param {Number} [height=3] Height of the pick rectangle.

* @returns {Object} Object containing the picked primitive.
*
* @exception {DeveloperError} windowPosition is undefined.
*/
Scene.prototype.pick = function(windowPosition) {
Scene.prototype.pick = function(windowPosition, width, height) {
//>>includeStart('debug', pragmas.debug);
if(!defined(windowPosition)) {
throw new DeveloperError('windowPosition is undefined.');
}
//>>includeEnd('debug');
// override the rectangle dimensions if defined
var rectangleWidth = width || 3.0;
var rectangleHeight = height || width || 3.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use defaultValue here:

var rectangleWidth = defaultValue(width, 3.0);
var rectangleHeight = defaultValue(height, rectangleWidth);

Also the comment above can be removed.

var scratchRectangle = new BoundingRectangle(0.0, 0.0, rectangleWidth, rectangleHeight);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is still worth storing the scratch rectangle in file scope so that a new rectangle isn't allocated for every pick call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only thought on using the scratch rectangle is that it will modify the default size for future calls to pick. So if you decided to call pick and change the size to say 10, from there on the pick routine will use 10 instead of 3 until another call to it is made that changes the size. I suppose I could reset it back to the default at the end of the routine...

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to how scratchRectangle.x and .y are set always, you could set .width and .height right below.


var context = this._context;
var us = context.uniformState;
Expand All @@ -2825,7 +2828,6 @@ define([

scratchRectangle.x = drawingBufferPosition.x - ((rectangleWidth - 1.0) * 0.5);
scratchRectangle.y = (this.drawingBufferHeight - drawingBufferPosition.y) - ((rectangleHeight - 1.0) * 0.5);

var passState = this._pickFramebuffer.begin(scratchRectangle);

updateEnvironment(this, passState);
Expand Down
47 changes: 46 additions & 1 deletion Specs/Scene/PickSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ defineSuite([
'Scene/PerspectiveFrustum',
'Scene/Primitive',
'Scene/SceneMode',
'Specs/createScene'
'Specs/createCanvas',
'Specs/createScene',
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comma.

], 'Scene/Pick', function(
FeatureDetection,
GeometryInstance,
Expand All @@ -23,6 +24,7 @@ defineSuite([
PerspectiveFrustum,
Primitive,
SceneMode,
createCanvas,
createScene) {
'use strict';

Expand Down Expand Up @@ -58,6 +60,20 @@ defineSuite([
primitives.removeAll();
});

// reset the scene such that the canvas is 10 pixels and the camera is set
// such that the primitive only takes up a small area at the canvas center
function setupPickSizeTestScene() {
scene = createScene({canvas: createCanvas(10,10) });
primitives = scene.primitives;
camera = scene.camera;
scene.mode = SceneMode.SCENE3D;
scene.morphTime = SceneMode.getMorphTime(scene.mode);

camera.setView({
destination : Rectangle.fromDegrees(-10.0, -10.0, 10.0, 10.0)
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just always construct scene with a 10x10 canvas in beforeAll and add the camera adjustment to the individual tests. This will avoid the scene creation boilerplate.


function createRectangle() {
var e = new Primitive({
geometryInstances: new GeometryInstance({
Expand Down Expand Up @@ -94,6 +110,35 @@ defineSuite([
expect(scene).toPickPrimitive(rectangle);
});

it('picks a primitive at pickSize extent', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

pickSize isn't a name used by the API. Maybe instead picks a primitive with a given width and height.

if (FeatureDetection.isInternetExplorer()) {
// Workaround IE 11.0.9. This test fails when all tests are ran without a breakpoint here.
return;
}

setupPickSizeTestScene();
var rectangle = createRectangle();
// scene is 10px, primitive is at center, pick size convers 5px pixel from corner
var config = { x: 0, y: 0, size: 5 };

expect(scene).toPickNearbyPrimitive(rectangle, config);
});

it('does not pick a primitive at pickSize extent', function() {
if (FeatureDetection.isInternetExplorer()) {
// Workaround IE 11.0.9. This test fails when all tests are ran without a breakpoint here.
return;
}

setupPickSizeTestScene();
createRectangle();
// scene is 10px, primitive is at center, pick size only convers 3px from corner
var config = { x: 0, y: 0, size: 3 };

expect(scene).notToPickNearbyPrimitive(config);
});


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this extra line.

it('does not pick primitives when show is false', function() {
var rectangle = createRectangle();
rectangle.show = false;
Expand Down
31 changes: 29 additions & 2 deletions Specs/addDefaultMatchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,22 @@ define([
};
},

toPickNearbyPrimitive : function(util, customEqualityTesters) {
return {
compare : function(actual, expected, config) {
return pickPrimitiveEqualsWrapper(actual, expected, config.x, config.y, config.size);
}
};
},

notToPickNearbyPrimitive : function(util, customEqualityTesters) {
return {
compare : function(actual, config) {
return pickPrimitiveEqualsWrapper(actual, undefined, config.x, config.y, config.size);
}
};
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding these two perhaps just add the functionality to toPickPrimitive and notToPick.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it would be fine to send in the values as separate arguments instead of a config object.


notToPick : function(util, customEqualityTesters) {
return {
compare : function(actual, expected) {
Expand Down Expand Up @@ -416,6 +432,17 @@ define([
};
}

// converts x and y coordinates such that after updating for drawing buffer position, the correct coordinates will be used
function pickPrimitiveEqualsWrapper(actual, expected, x, y, width, height) {
width = width || 3;
height = height || width || 3;
x = x || 0;
y = y || 0;
var adjustedX = x + ((width - 1) * 0.5);
var adjustedY = -1 * (y + ((height - 1) * 0.5) - actual._context.drawingBufferHeight);
return pickPrimitiveEquals(actual, expected, adjustedX, adjustedY, width, height);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not completely following the conversion process here. Can the test be modified instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this conversion is the inverse of

scratchRectangle.x = drawingBufferPosition.x - ((rectangleWidth - 1.0) * 0.5);
scratchRectangle.y = (this.drawingBufferHeight - drawingBufferPosition.y) - ((rectangleHeight - 1.0) * 0.5);

from lines 2829 in Scene.js

Honestly, I don't understand what this logic is accomplishing, but for the test I need to set the bounding box for the pick search to be centered at a position exactly far enough from the rectangle for the increase in pick size to affect the outcome. This was just something that would never work right until I implemented this logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see now. By changing to test to pass in x and y values other than 0 should also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you saying this conversion is unnecessary when the x and y values aren't 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I tried x = 7 and y = 7 and that seems to work.

}

function renderAndReadPixels(options) {
var scene;

Expand Down Expand Up @@ -465,9 +492,9 @@ define([
};
}

function pickPrimitiveEquals(actual, expected) {
function pickPrimitiveEquals(actual, expected, x, y, pickWidth, pickHeight) {
var scene = actual;
var result = scene.pick(new Cartesian2(0, 0));
var result = scene.pick(new Cartesian2(x || 0, y || 0), pickWidth, pickHeight);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also use defaultValue here.


if (!!window.webglStub) {
return {
Expand Down