Picking tiles without batch tables #5253

rahwang · 2017-04-28T15:14:35Z

Fixes #2181. How should I test this, @lilleyse ?

lilleyse · 2017-04-28T18:03:34Z

Source/Scene/Batched3DModel3DTileContent.js

@@ -362,7 +362,8 @@ define([
            pickVertexShaderLoaded : getPickVertexShaderCallback(content),
            pickFragmentShaderLoaded : batchTable.getPickFragmentShaderCallback(),
            pickUniformMapLoaded : batchTable.getPickUniformMapCallback(),
-            addBatchIdToGeneratedShaders : (batchLength > 0) // If the batch table has values in it, generated shaders will need a batchId attribute
+            addBatchIdToGeneratedShaders : (batchLength > 0), // If the batch table has values in it, generated shaders will need a batchId attribute
+            pickInfo : {tile : content._tile}


Remove all occurrences of pickPrimitive now that there's pickInfo. Also pickObject may be a better name now.

Also include primitive (= tileset) and content into the pick object. Create the pick object as a variable above so this area is cleaner.

lilleyse · 2017-04-28T18:06:37Z

Source/Scene/Cesium3DTileFeature.js

@@ -46,7 +46,7 @@ define([
        this._batchTable = content.batchTable;
        this._batchId = batchId;
        this._color = undefined;  // for calling getColor
-
+        this._tile = content._tile;


_tile and _content will need to lose the underscores to be consistent with the pick object sent to Model and the pick object in PointCloud3DTileContent. Add doc above them like for this.primitive.

lilleyse · 2017-04-28T18:07:58Z

Source/Scene/Model.js

@@ -3468,6 +3469,10 @@ define([
                    mesh : runtimeMeshesByName[mesh.name]
                };

+                if (defined(model._pickInfo)) {
+                    owner = combine(owner, model._pickInfo);
+                }


I'm starting to rethink this. If the custom pick object exists then it should be used directly, not combined with owner. I doubt anyone picking a tile needs to know what gltf node/mesh are picked.

lilleyse · 2017-04-28T18:20:26Z

How should I test this, @lilleyse ?

You can look at picks in PointCloud3DTileContent as an example. That test needs to be modified to check that result.tile is there.

For Batched3DTileContent make sure the tileset you load is withoutBatchTableUrl.

For more visual testing, you can add some picking code to the 3D Tiles sandcastle and print the results, checking that they look right for a bunch of different tileset urls.

rahwang · 2017-05-02T21:49:51Z

Comments addressed and added tests, @lilleyse

lilleyse · 2017-05-03T14:44:14Z

Source/Scene/Cesium3DTileFeature.js

@@ -55,6 +53,24 @@ define([
         * @private
         */
        this.primitive = tileset;
+
+        /**
+         * All objects returned by {@link Scene#pick} have a <code>tile</code> property.


Replace this comment and the one below with something else, since this is not true for non 3d-tiles things. The comment above primitive is still correct though.

lilleyse · 2017-05-03T14:45:27Z

Source/Scene/Cesium3DTileFeature.js

+        /**
+         * All objects returned by {@link Scene#pick} have a <code>tile</code> property.
+         *
+         * @type {Cesium3DTileset}


Change the type.

lilleyse · 2017-05-03T14:45:34Z

Source/Scene/Cesium3DTileFeature.js

+        /**
+         * All objects returned by {@link Scene#pick} have a <code>content</code> property.
+         *
+         * @type {Cesium3DTileset}


Change the type.

lilleyse · 2017-05-03T14:49:47Z

Source/Scene/Model.js

@@ -3462,12 +3461,16 @@ define([
                // GLTF_SPEC: Offical means to determine translucency. https://github.com/KhronosGroup/glTF/issues/105
                var isTranslucent = rs.blending.enabled;
                var owner = {
-                    primitive : defaultValue(model.pickPrimitive, model),
+                    primitive : defaultValue(model._pickObject, model),


Just set primitive to model.

lilleyse · 2017-05-03T14:49:58Z

Source/Scene/Model.js

                    id : model.id,
                    node : runtimeNode.publicNode,
                    mesh : runtimeMeshesByName[mesh.name]
                };

+                if (defined(model._pickObject)) {
+                    owner = model._pickObject;
+                }


The logic here can be reversed to avoid the owner allocation. Something like

var owner = model._pickObject; if (!defined(owner)) { owner = ... }

lilleyse · 2017-05-03T14:51:27Z

Specs/Scene/Batched3DModel3DTileContentSpec.js

+                expect(result.tile).toBe(content._tile);
+            });
+        });
+    });


These tests look good!

rahwang · 2017-05-03T16:09:33Z

Comments addressed! Ready for another look, @lilleyse

lilleyse · 2017-05-03T16:13:54Z

Looks good!

pjcozzi · 2017-05-03T17:58:38Z

Source/Scene/Cesium3DTileFeature.js

+         *
+         * @private
+         */
+        this.tile = content._tile;


Should this be a readonly getter property?

Also, are we sure we need this property? There will be a lot of these objects since they are per-feature and we want to keep them as small as possible.

Technically we don't, this was mainly for convenience.

I don't think it is worth it since this can be accessed through content and I think was only used in the tests.

pjcozzi · 2017-05-03T18:00:53Z

Source/Scene/Cesium3DTileFeature.js

+         *
+         * @private
+         */
+        this.content = content;


Why isn't this readonly anymore? Is there something outside of this PR?

adding tile to tilefeature and pickId objects

9238239

rahwang changed the title ~~adding tile to tilefeature and pickId objects~~ Picking tiles without batch tables Apr 28, 2017

lilleyse reviewed Apr 28, 2017

View reviewed changes

switching to pickObject

b3f8aed

rahwang force-pushed the picking-without-batch-tables branch from 1a13884 to 5d0cd81 Compare May 2, 2017 21:49

edits and tests for tile picking

ec24907

rahwang force-pushed the picking-without-batch-tables branch from 5d0cd81 to ec24907 Compare May 3, 2017 13:23

lilleyse reviewed May 3, 2017

View reviewed changes

edits

5942696

lilleyse merged commit 3a3c466 into CesiumGS:3d-tiles May 3, 2017

pjcozzi reviewed May 3, 2017

View reviewed changes

rahwang mentioned this pull request May 10, 2017

cleanup tile picking without batch table #5305

Merged

rahwang deleted the picking-without-batch-tables branch June 26, 2017 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Picking tiles without batch tables #5253

Picking tiles without batch tables #5253

rahwang commented Apr 28, 2017 •

edited

Loading

lilleyse Apr 28, 2017

lilleyse Apr 28, 2017

lilleyse Apr 28, 2017 •

edited

Loading

lilleyse commented Apr 28, 2017 •

edited

Loading

rahwang commented May 2, 2017

lilleyse May 3, 2017

lilleyse May 3, 2017

lilleyse May 3, 2017

lilleyse May 3, 2017

lilleyse May 3, 2017

lilleyse May 3, 2017

rahwang commented May 3, 2017

lilleyse commented May 3, 2017

pjcozzi May 3, 2017

pjcozzi May 3, 2017

lilleyse May 3, 2017

pjcozzi May 3, 2017

pjcozzi May 3, 2017

Picking tiles without batch tables #5253

Picking tiles without batch tables #5253

Conversation

rahwang commented Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

lilleyse commented Apr 28, 2017 • edited Loading

rahwang commented May 2, 2017

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

rahwang commented May 3, 2017

lilleyse commented May 3, 2017

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

rahwang commented Apr 28, 2017 •

edited

Loading

lilleyse Apr 28, 2017 •

edited

Loading

lilleyse commented Apr 28, 2017 •

edited

Loading