Merge pull request #7113 from AnalyticalGraphicsInc/fix-pick-position

Fix pickPosition called after sampleHeight

Author: bagnell

CHANGES.md

@@ -7,6 +7,9 @@ Change Log
 * Added WMS-T (time) support in WebMapServiceImageryProvider [#2581](https://github.com/AnalyticalGraphicsInc/cesium/issues/2581)
 * Added `cutoutRectangle` to `ImageryLayer`, which allows cutting out rectangular areas in imagery layers to reveal underlying imagery. [#7056](https://github.com/AnalyticalGraphicsInc/cesium/pull/7056)
 
+##### Fixes :wrench:
+* Fixed an issue where `pickPosition` would return incorrect results when called after `sampleHeight` or `clampToHeight`. [#7113](https://github.com/AnalyticalGraphicsInc/cesium/pull/7113)
+
 ### 1.50 - 2018-10-01
 
 ##### Breaking Changes :mega:

Source/Core/Transforms.js

@@ -120,94 +120,94 @@ define([
     * @return {localFrameToFixedFrameGenerator~resultat} The function that will computes a
     * 4x4 transformation matrix from a reference frame, with first axis and second axis compliant with the parameters,
     */
-    Transforms.localFrameToFixedFrameGenerator = function( firstAxis, secondAxis) {
-      if (!vectorProductLocalFrame.hasOwnProperty(firstAxis) || !vectorProductLocalFrame[firstAxis].hasOwnProperty(secondAxis)) {
-          throw new DeveloperError('firstAxis and secondAxis must be east, north, up, west, south or down.');
-      }
-      var thirdAxis = vectorProductLocalFrame[firstAxis][secondAxis];
-
-      /**
-       * Computes a 4x4 transformation matrix from a reference frame
-       * centered at the provided origin to the provided ellipsoid's fixed reference frame.
-       * @callback Transforms~LocalFrameToFixedFrame
-       * @param {Cartesian3} origin The center point of the local reference frame.
-       * @param {Ellipsoid} [ellipsoid=Ellipsoid.WGS84] The ellipsoid whose fixed frame is used in the transformation.
-       * @param {Matrix4} [result] The object onto which to store the result.
-       * @returns {Matrix4} The modified result parameter or a new Matrix4 instance if none was provided.
-       */
-      var resultat;
-      var hashAxis = firstAxis + secondAxis;
-      if (defined(localFrameToFixedFrameCache[hashAxis])) {
-          resultat = localFrameToFixedFrameCache[hashAxis];
-      } else {
-          resultat = function(origin, ellipsoid, result) {
-              //>>includeStart('debug', pragmas.debug);
-              if (!defined(origin)) {
-                  throw new DeveloperError('origin is required.');
-              }
-              //>>includeEnd('debug');
-              if (!defined(result)) {
-                  result = new Matrix4();
-              }
-              // If x and y are zero, assume origin is at a pole, which is a special case.
-              if (CesiumMath.equalsEpsilon(origin.x, 0.0, CesiumMath.EPSILON14) && CesiumMath.equalsEpsilon(origin.y, 0.0, CesiumMath.EPSILON14)) {
-                  var sign = CesiumMath.sign(origin.z);
-
-                  Cartesian3.unpack(degeneratePositionLocalFrame[firstAxis], 0, scratchFirstCartesian);
-                  if (firstAxis !== 'east' && firstAxis !== 'west') {
-                      Cartesian3.multiplyByScalar(scratchFirstCartesian, sign, scratchFirstCartesian);
-                  }
-
-                  Cartesian3.unpack(degeneratePositionLocalFrame[secondAxis], 0, scratchSecondCartesian);
-                  if (secondAxis !== 'east' && secondAxis !== 'west') {
-                      Cartesian3.multiplyByScalar(scratchSecondCartesian, sign, scratchSecondCartesian);
-                  }
-
-                  Cartesian3.unpack(degeneratePositionLocalFrame[thirdAxis], 0, scratchThirdCartesian);
-                  if (thirdAxis !== 'east' && thirdAxis !== 'west') {
-                      Cartesian3.multiplyByScalar(scratchThirdCartesian, sign, scratchThirdCartesian);
-                  }
-              } else {
-                  ellipsoid = defaultValue(ellipsoid, Ellipsoid.WGS84);
-                  ellipsoid.geodeticSurfaceNormal(origin, scratchCalculateCartesian.up);
-
-                  var up = scratchCalculateCartesian.up;
-                  var east = scratchCalculateCartesian.east;
-                  east.x = -origin.y;
-                  east.y = origin.x;
-                  east.z = 0.0;
-                  Cartesian3.normalize(east, scratchCalculateCartesian.east);
-                  Cartesian3.cross(up, east, scratchCalculateCartesian.north);
-
-                  Cartesian3.multiplyByScalar(scratchCalculateCartesian.up, -1, scratchCalculateCartesian.down);
-                  Cartesian3.multiplyByScalar(scratchCalculateCartesian.east, -1, scratchCalculateCartesian.west);
-                  Cartesian3.multiplyByScalar(scratchCalculateCartesian.north, -1, scratchCalculateCartesian.south);
-
-                  scratchFirstCartesian = scratchCalculateCartesian[firstAxis];
-                  scratchSecondCartesian = scratchCalculateCartesian[secondAxis];
-                  scratchThirdCartesian = scratchCalculateCartesian[thirdAxis];
-              }
-              result[0] = scratchFirstCartesian.x;
-              result[1] = scratchFirstCartesian.y;
-              result[2] = scratchFirstCartesian.z;
-              result[3] = 0.0;
-              result[4] = scratchSecondCartesian.x;
-              result[5] = scratchSecondCartesian.y;
-              result[6] = scratchSecondCartesian.z;
-              result[7] = 0.0;
-              result[8] = scratchThirdCartesian.x;
-              result[9] = scratchThirdCartesian.y;
-              result[10] = scratchThirdCartesian.z;
-              result[11] = 0.0;
-              result[12] = origin.x;
-              result[13] = origin.y;
-              result[14] = origin.z;
-              result[15] = 1.0;
-              return result;
-          };
-          localFrameToFixedFrameCache[hashAxis] = resultat;
-      }
-      return resultat;
+    Transforms.localFrameToFixedFrameGenerator = function (firstAxis, secondAxis) {
+        if (!vectorProductLocalFrame.hasOwnProperty(firstAxis) || !vectorProductLocalFrame[firstAxis].hasOwnProperty(secondAxis)) {
+            throw new DeveloperError('firstAxis and secondAxis must be east, north, up, west, south or down.');
+        }
+        var thirdAxis = vectorProductLocalFrame[firstAxis][secondAxis];
+
+        /**
+         * Computes a 4x4 transformation matrix from a reference frame
+         * centered at the provided origin to the provided ellipsoid's fixed reference frame.
+         * @callback Transforms~LocalFrameToFixedFrame
+         * @param {Cartesian3} origin The center point of the local reference frame.
+         * @param {Ellipsoid} [ellipsoid=Ellipsoid.WGS84] The ellipsoid whose fixed frame is used in the transformation.
+         * @param {Matrix4} [result] The object onto which to store the result.
+         * @returns {Matrix4} The modified result parameter or a new Matrix4 instance if none was provided.
+         */
+        var resultat;
+        var hashAxis = firstAxis + secondAxis;
+        if (defined(localFrameToFixedFrameCache[hashAxis])) {
+            resultat = localFrameToFixedFrameCache[hashAxis];
+        } else {
+            resultat = function (origin, ellipsoid, result) {
+                //>>includeStart('debug', pragmas.debug);
+                if (!defined(origin)) {
+                    throw new DeveloperError('origin is required.');
+                }
+                //>>includeEnd('debug');
+                if (!defined(result)) {
+                    result = new Matrix4();
+                }
+                // If x and y are zero, assume origin is at a pole, which is a special case.
+                if (CesiumMath.equalsEpsilon(origin.x, 0.0, CesiumMath.EPSILON14) && CesiumMath.equalsEpsilon(origin.y, 0.0, CesiumMath.EPSILON14)) {
+                    var sign = CesiumMath.sign(origin.z);
+
+                    Cartesian3.unpack(degeneratePositionLocalFrame[firstAxis], 0, scratchFirstCartesian);
+                    if (firstAxis !== 'east' && firstAxis !== 'west') {
+                        Cartesian3.multiplyByScalar(scratchFirstCartesian, sign, scratchFirstCartesian);
+                    }
+
+                    Cartesian3.unpack(degeneratePositionLocalFrame[secondAxis], 0, scratchSecondCartesian);
+                    if (secondAxis !== 'east' && secondAxis !== 'west') {
+                        Cartesian3.multiplyByScalar(scratchSecondCartesian, sign, scratchSecondCartesian);
+                    }
+
+                    Cartesian3.unpack(degeneratePositionLocalFrame[thirdAxis], 0, scratchThirdCartesian);
+                    if (thirdAxis !== 'east' && thirdAxis !== 'west') {
+                        Cartesian3.multiplyByScalar(scratchThirdCartesian, sign, scratchThirdCartesian);
+                    }
+                } else {
+                    ellipsoid = defaultValue(ellipsoid, Ellipsoid.WGS84);
+                    ellipsoid.geodeticSurfaceNormal(origin, scratchCalculateCartesian.up);
+
+                    var up = scratchCalculateCartesian.up;
+                    var east = scratchCalculateCartesian.east;
+                    east.x = -origin.y;
+                    east.y = origin.x;
+                    east.z = 0.0;
+                    Cartesian3.normalize(east, scratchCalculateCartesian.east);
+                    Cartesian3.cross(up, east, scratchCalculateCartesian.north);
+
+                    Cartesian3.multiplyByScalar(scratchCalculateCartesian.up, -1, scratchCalculateCartesian.down);
+                    Cartesian3.multiplyByScalar(scratchCalculateCartesian.east, -1, scratchCalculateCartesian.west);
+                    Cartesian3.multiplyByScalar(scratchCalculateCartesian.north, -1, scratchCalculateCartesian.south);
+
+                    scratchFirstCartesian = scratchCalculateCartesian[firstAxis];
+                    scratchSecondCartesian = scratchCalculateCartesian[secondAxis];
+                    scratchThirdCartesian = scratchCalculateCartesian[thirdAxis];
+                }
+                result[0] = scratchFirstCartesian.x;
+                result[1] = scratchFirstCartesian.y;
+                result[2] = scratchFirstCartesian.z;
+                result[3] = 0.0;
+                result[4] = scratchSecondCartesian.x;
+                result[5] = scratchSecondCartesian.y;
+                result[6] = scratchSecondCartesian.z;
+                result[7] = 0.0;
+                result[8] = scratchThirdCartesian.x;
+                result[9] = scratchThirdCartesian.y;
+                result[10] = scratchThirdCartesian.z;
+                result[11] = 0.0;
+                result[12] = origin.x;
+                result[13] = origin.y;
+                result[14] = origin.z;
+                result[15] = 1.0;
+                return result;
+            };
+            localFrameToFixedFrameCache[hashAxis] = resultat;
+        }
+        return resultat;
     };
 
     /**

Source/Scene/Scene.js

@@ -3400,6 +3400,7 @@ define([
             return Cartesian3.clone(this._pickPositionCache[cacheKey], result);
         }
 
+        var frameState = this._frameState;
         var context = this._context;
         var uniformState = context.uniformState;
 
@@ -3410,6 +3411,8 @@ define([
         if (this.pickTranslucentDepth) {
             renderTranslucentDepthForPick(this, drawingBufferPosition);
         } else {
+            updateFrameState(this, frameState.frameNumber, frameState.time);
+            uniformState.update(frameState);
             updateEnvironment(this);
         }
         drawingBufferPosition.y = this.drawingBufferHeight - drawingBufferPosition.y;
@@ -3441,7 +3444,7 @@ define([
                     camera.position.z = height2D - renderedFrustum.near + 1.0;
                     frustum.far = Math.max(1.0, renderedFrustum.far - renderedFrustum.near);
                     frustum.near = 1.0;
-                    uniformState.update(this.frameState);
+                    uniformState.update(frameState);
                     uniformState.updateFrustum(frustum);
                 } else {
                     frustum.near = renderedFrustum.near * (i !== 0 ? this.opaqueFrustumNearOffset : 1.0);
@@ -3453,7 +3456,7 @@ define([
 
                 if (this.mode === SceneMode.SCENE2D) {
                     camera.position.z = height2D;
-                    uniformState.update(this.frameState);
+                    uniformState.update(frameState);
                 }
 
                 this._pickPositionCache[cacheKey] = Cartesian3.clone(result);

Specs/Scene/PickSpec.js

@@ -1,6 +1,7 @@
 defineSuite([
         'Core/Cartesian3',
         'Core/Cartographic',
+        'Core/Color',
         'Core/Ellipsoid',
         'Core/FeatureDetection',
         'Core/GeometryInstance',
@@ -27,6 +28,7 @@ defineSuite([
     ], 'Scene/Pick', function(
         Cartesian3,
         Cartographic,
+        Color,
         Ellipsoid,
         FeatureDetection,
         GeometryInstance,
@@ -57,7 +59,7 @@ defineSuite([
     var camera;
     var largeRectangle = Rectangle.fromDegrees(-1.0, -1.0, 1.0, 1.0);
     var smallRectangle = Rectangle.fromDegrees(-0.0001, -0.0001, 0.0001, 0.0001);
-    var offscreenRectangle = Rectangle.fromDegrees(-45.0, -1.0, -43.0, 1.0);
+    var offscreenRectangle = Rectangle.fromDegrees(-45.0002, -1.0002, -45.0001, -1.0001);
     var primitiveRay;
     var offscreenRay;
 
@@ -1081,4 +1083,69 @@ defineSuite([
             scene.context._depthTexture = depthTexture;
         });
     });
+
+    it('calls multiple picking functions within the same frame', function() {
+        if (!scene.clampToHeightSupported || !scene.pickPositionSupported) {
+            return;
+        }
+
+        createSmallRectangle(0.0);
+        var offscreenRectanglePrimitive = createRectangle(0.0, offscreenRectangle);
+        offscreenRectanglePrimitive.appearance.material.uniforms.color = new Color(1.0, 0.0, 0.0, 1.0);
+
+        scene.camera.setView({ destination : offscreenRectangle });
+
+        // Call render. Lays down depth for the pickPosition call
+        scene.renderForSpecs();
+
+        // Call clampToHeight
+        var cartesian = Cartesian3.fromRadians(0.0, 0.0, 100000.0);
+        expect(scene).toClampToHeightAndCall(function(cartesian) {
+            var expectedCartesian = Cartesian3.fromRadians(0.0, 0.0);
+            expect(cartesian).toEqualEpsilon(expectedCartesian, CesiumMath.EPSILON5);
+        }, cartesian);
+
+        // Call pickPosition
+        expect(scene).toPickPositionAndCall(function(cartesian) {
+            var expectedCartesian = Cartographic.toCartesian(Rectangle.center(offscreenRectangle));
+            expect(cartesian).toEqualEpsilon(expectedCartesian, CesiumMath.EPSILON5);
+        });
+
+        // Call clampToHeight again
+        expect(scene).toClampToHeightAndCall(function(cartesian) {
+            var expectedCartesian = Cartesian3.fromRadians(0.0, 0.0);
+            expect(cartesian).toEqualEpsilon(expectedCartesian, CesiumMath.EPSILON5);
+        }, cartesian);
+
+        // Call pick
+        expect(scene).toPickPrimitive(offscreenRectanglePrimitive);
+
+        // Call clampToHeight again
+        expect(scene).toClampToHeightAndCall(function(cartesian) {
+            var expectedCartesian = Cartesian3.fromRadians(0.0, 0.0);
+            expect(cartesian).toEqualEpsilon(expectedCartesian, CesiumMath.EPSILON5);
+        }, cartesian);
+
+        // Call pickPosition on translucent primitive and returns undefined
+        offscreenRectanglePrimitive.appearance.material.uniforms.color = new Color(1.0, 0.0, 0.0, 0.5);
+        scene.renderForSpecs();
+        expect(scene).toPickPositionAndCall(function(cartesian) {
+            expect(cartesian).toBeUndefined();
+        });
+
+        // Call clampToHeight again
+        expect(scene).toClampToHeightAndCall(function(cartesian) {
+            var expectedCartesian = Cartesian3.fromRadians(0.0, 0.0);
+            expect(cartesian).toEqualEpsilon(expectedCartesian, CesiumMath.EPSILON5);
+        }, cartesian);
+
+        // Call pickPosition on translucent primitive with pickTranslucentDepth
+        scene.pickTranslucentDepth = true;
+        scene.renderForSpecs();
+        expect(scene).toPickPositionAndCall(function(cartesian) {
+            var expectedCartesian = Cartographic.toCartesian(Rectangle.center(offscreenRectangle));
+            expect(cartesian).toEqualEpsilon(expectedCartesian, CesiumMath.EPSILON5);
+        });
+    });
+
 }, 'WebGL');

Specs/addDefaultMatchers.js

@@ -439,6 +439,30 @@ define([
                 };
             },
 
+            toPickPositionAndCall : function(util, customEqualityTesters) {
+                return {
+                    compare : function(actual, expected, x, y) {
+                        var scene = actual;
+                        var canvas = scene.canvas;
+                        x = defaultValue(x, canvas.clientWidth / 2);
+                        y = defaultValue(y, canvas.clientHeight / 2);
+                        var result = scene.pickPosition(new Cartesian2(x, y));
+
+                        var webglStub = !!window.webglStub;
+                        if (!webglStub) {
+                            // The callback may have expectations that fail, which still makes the
+                            // spec fail, as we desired, even though this matcher sets pass to true.
+                            var callback = expected;
+                            callback(result);
+                        }
+
+                        return {
+                            pass : true
+                        };
+                    }
+                };
+            },
+
             toReadPixels : function(util, customEqualityTesters) {
                 return {
                     compare : function(actual, expected) {