Added feature to modify scene pick search size #5602

jason-crow · 2017-07-10T14:50:43Z

This feature adds two optional arguments to the scene pick method, width and height, which allows for modifying the bounding box used to search for matching primitives at the provided window position. If only width is provided, the height is set to the value provided for width. To verify this feature, I added two tests, one to pick a primitive at the extent of the pick size and one to fail to pick the primitive when the pick size is reduced from the same window position.

This feature makes it easier to select narrow primitives such as wireframes in a 2d scene.

… method to accomodate a feature for modifying the size of the pick aperture. This, for example, makes it easier to select wireframe elements or other similarly narrow tileset elements by providing a means to increase the box volume used to pick an intersecting primitive.

hpinkos · 2017-07-10T14:54:31Z

Thanks for the pull request @jason-crow! @bagnell, do you want to review this?

hpinkos · 2017-07-10T14:55:31Z

CONTRIBUTORS.md

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


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

Oops, updated

jason-crow · 2017-07-10T15:05:44Z

Yes please From: Hannah [mailto:notifications@github.com] Sent: Monday, July 10, 2017 9:55 AM To: AnalyticalGraphicsInc/cesium <cesium@noreply.github.com> Cc: Jason Crow <Jason.Crow@bentley.com>; Mention <mention@noreply.github.com> Subject: Re: [AnalyticalGraphicsInc/cesium] Added feature to modify scene pick search size (#5602) Thanks for the pull request @jason-crow<https://github.com/jason-crow>! @bagnell<https://github.com/bagnell>, do you want to review this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#5602 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AYoEfquMhTuE8Ft3WIKU5Kxf27MRhangks5sMjswgaJpZM4OS9Nl>.

lilleyse · 2017-07-11T13:49:05Z

Source/Scene/Scene.js

+        // override the rectangle dimensions if defined
+        var rectangleWidth = width || 3.0;
+        var rectangleHeight = height || width || 3.0;
+        var scratchRectangle = new BoundingRectangle(0.0, 0.0, rectangleWidth, rectangleHeight);


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

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

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

lilleyse · 2017-07-11T13:49:07Z

Source/Scene/Scene.js

        //>>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;


Use defaultValue here:

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

Also the comment above can be removed.

lilleyse · 2017-07-11T13:49:10Z

Source/Scene/Scene.js

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


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.

lilleyse · 2017-07-11T13:55:30Z

Specs/addDefaultMatchers.js

        var scene = actual;
-        var result = scene.pick(new Cartesian2(0, 0));
+        var result = scene.pick(new Cartesian2(x || 0, y || 0), pickWidth, pickHeight);


Also use defaultValue here.

lilleyse · 2017-07-11T14:11:40Z

Specs/Scene/PickSpec.js

+        camera.setView({
+            destination : Rectangle.fromDegrees(-10.0, -10.0, 10.0, 10.0)
+        });
+    }


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.

lilleyse · 2017-07-11T14:14:09Z

Specs/addDefaultMatchers.js

+                        return pickPrimitiveEqualsWrapper(actual, undefined, config.x, config.y, config.size);
+                    }
+                };
+            },


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

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

lilleyse · 2017-07-11T14:19:43Z

Specs/Scene/PickSpec.js

@@ -94,6 +110,35 @@ defineSuite([
        expect(scene).toPickPrimitive(rectangle);
    });

+    it('picks a primitive at pickSize extent', function() {


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

lilleyse · 2017-07-11T14:20:32Z

Specs/addDefaultMatchers.js

+        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);


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

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.

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

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

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

lilleyse · 2017-07-12T14:21:05Z

@jason-crow is this ready for another pass?

jason-crow · 2017-07-12T14:24:05Z

Yes

lilleyse

Thanks for the updates @jason-crow! I also added some comments for things I missed on the first go.

lilleyse · 2017-07-13T15:04:13Z

Source/Scene/Scene.js

+        // override the rectangle dimensions if defined
+        rectangleWidth = defaultValue(width, 3.0);
+        rectangleHeight = defaultValue(height, rectangleWidth);
+        scratchRectangle = new BoundingRectangle(0.0, 0.0, rectangleWidth, rectangleHeight);


Check out my latest comment here: #5602 (comment). I think we can get away without allocating the scratch rectangle in the function.

lilleyse · 2017-07-13T15:13:44Z

Specs/addDefaultMatchers.js

        var scene = actual;
-        var result = scene.pick(new Cartesian2(0, 0));
+        var windowPosition = new Cartesian2(defaultValue(x,0), defaultValue(y,0));


Cartesian2 defaults to 0.0 by default when arguments are undefiend, so just new Cartesian2(x, y) should work.

lilleyse · 2017-07-13T15:15:07Z

Specs/createScene.js

@@ -20,7 +20,7 @@ define([
        options = defaultValue(options, {});

        // save the canvas so we don't try to clone an HTMLCanvasElement
-        var canvas = defined(options.canvas) ? options.canvas : createCanvas();
+        var canvas = defined(options.canvas) ? options.canvas : createCanvas(10,10);


Instead of the default of (10, 10) here, PickSpec should instead send in a 10x10 canvas.

lilleyse · 2017-07-13T15:15:59Z

Specs/Scene/PickSpec.js

@@ -10,7 +10,7 @@ defineSuite([
        'Scene/PerspectiveFrustum',
        'Scene/Primitive',
        'Scene/SceneMode',
-        'Specs/createScene'
+        'Specs/createScene',


Remove comma.

lilleyse · 2017-07-13T15:16:56Z

Specs/addDefaultMatchers.js

-                    compare : function(actual, expected) {
-                        return pickPrimitiveEquals(actual, undefined);
+                    compare : function(actual, expected, x, y, size) {
+                        return pickPrimitiveEquals(actual, undefined, x, y, size);


For these two send in width and height instead of just size for consistency.

lilleyse · 2017-07-13T15:18:30Z

Specs/Scene/PickSpec.js

+        expect(scene).notToPick(0,0,3);
+    });
+
+


Remove this extra line.

lilleyse · 2017-07-13T15:19:59Z

Specs/Scene/PickSpec.js

+
+        createRectangle();
+
+        expect(scene).notToPick(0,0,3);


I'm starting to think this test and the one above can be combined into a single test, with both pick checks at the bottom.

it('picks a primitive with a modified pick search area', function() { if (FeatureDetection.isInternetExplorer()) { // Workaround IE 11.0.9. This test fails when all tests are ran without a breakpoint here. return; } camera.setView({ destination : Rectangle.fromDegrees(-10.0, -10.0, 10.0, 10.0) }); var rectangle = createRectangle(); expect(scene).toPickPrimitive(rectangle, 7, 7, 5); expect(scene).notToPick(7, 7, 3); });

I changed the numbers for the notToPick test also to (7, 7) for consistency.

jason-crow · 2017-07-13T18:24:43Z

Ok ready again, thanks for help

lilleyse · 2017-07-13T19:35:47Z

I seemed to have accidentally closed this while trying to push some changes to your branch, but also reset your branch to what's on master in the process... I'm not sure why it let me merge master but not push the other commits I had.

The code is safely on the pick-size branch. Could you merge that branch into your master branch and then push? I can't seem to do that, I'm getting permission denied errors.

Sorry for the hassle.

…cesium

jason-crow · 2017-07-13T20:05:28Z

ok i think ive merged everything now.

lilleyse · 2017-07-13T21:22:26Z

Thanks @jason-crow, all good now!

jason-crow · 2017-07-13T21:26:18Z

Great! Thanks for your help in getting this merged!

jason-crow added 4 commits June 30, 2017 16:05

added test for scene pick size

cdebbd8

updated CONTRIBUTORS.md

153946c

updated addDefaultMatchers.js

0a7238f

hpinkos reviewed Jul 10, 2017

View reviewed changes

updated CONTRIBUTORS.md

2617c07

lilleyse reviewed Jul 11, 2017

View reviewed changes

jason-crow added 2 commits July 11, 2017 14:07

pull request update

368f5dd

reverted change to karma-main

fae7cdb

lilleyse reviewed Jul 13, 2017

View reviewed changes

update for pull request

c2c10b1

lilleyse added 3 commits July 13, 2017 15:18

Small style fixes

ea7e20b

Merge branch 'master' into HEAD

bf8386b

Updated CHANGES.md

1ae9bca

lilleyse closed this Jul 13, 2017

lilleyse force-pushed the master branch from c2c10b1 to c216514 Compare July 13, 2017 19:22

jason-crow added 2 commits July 13, 2017 14:57

Merge branch 'master' of https://github.com/jason-crow/cesium

6ce6080

Merge branch 'pick-size' of https://github.com/AnalyticalGraphicsInc/…

76b6e87

…cesium

lilleyse reopened this Jul 13, 2017

lilleyse merged commit 7498a24 into CesiumGS:master Jul 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added feature to modify scene pick search size #5602

Added feature to modify scene pick search size #5602

jason-crow commented Jul 10, 2017

hpinkos commented Jul 10, 2017

hpinkos Jul 10, 2017

jason-crow Jul 10, 2017

jason-crow commented Jul 10, 2017 via email

lilleyse Jul 11, 2017

jason-crow Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 11, 2017

jason-crow Jul 11, 2017

lilleyse Jul 11, 2017

jason-crow Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse commented Jul 12, 2017

jason-crow commented Jul 12, 2017

lilleyse left a comment

lilleyse Jul 13, 2017

lilleyse Jul 13, 2017

lilleyse Jul 13, 2017

lilleyse Jul 13, 2017

lilleyse Jul 13, 2017

lilleyse Jul 13, 2017

lilleyse Jul 13, 2017

jason-crow commented Jul 13, 2017

lilleyse commented Jul 13, 2017

jason-crow commented Jul 13, 2017

lilleyse commented Jul 13, 2017

jason-crow commented Jul 13, 2017

Added feature to modify scene pick search size #5602

Added feature to modify scene pick search size #5602

Conversation

jason-crow commented Jul 10, 2017

hpinkos commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-crow commented Jul 10, 2017 via email

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

Choose a reason for hiding this comment

lilleyse commented Jul 12, 2017

jason-crow commented Jul 12, 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

jason-crow commented Jul 13, 2017

lilleyse commented Jul 13, 2017

jason-crow commented Jul 13, 2017

lilleyse commented Jul 13, 2017

jason-crow commented Jul 13, 2017