Only show picked tile debug labels #5325

rahwang · 2017-05-15T19:28:31Z

An addition to #5188 , adds option for only displaying debug labels for picked tile in the inspector.

lilleyse

Looks good so far, a few things:

It is sometimes hard to see the info box when picking a tile that is close up. Something like the Picking sandcastle where the box follows the cursor would be better.
Picking doesn't work for tiles without a batch table, like ../../../Specs/Data/Cesium3DTiles/Batched/BatchedNoBatchIds
Add a test to Cesium3DTileset

lilleyse · 2017-05-16T14:48:03Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspector.js

@@ -196,6 +196,7 @@ define([
        errorBox.setAttribute('data-bind', 'text: editorError');
        stylePanelContents.appendChild(errorBox);

+        tileDebugLabelsPanelContents.appendChild(makeCheckbox('showOnlyPickedTileDebugLabel', 'Show Only Picked Tile Label'));


The text could be simplified to Show Picked Only

lilleyse · 2017-05-16T14:48:53Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

@@ -327,9 +327,11 @@ define([
                        var picked = scene.pick(e.endPosition);
                        if (picked instanceof Cesium3DTileFeature) {
                            that.feature = picked;
+                            that._tileset._pickedTile = picked.content._tile;


Rename to _debugPickedTile

lilleyse · 2017-05-16T15:01:47Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                showOnlyPickedTileDebugLabel(value);
+                if (that._tileset) {
+                    that._tileset._onlyPickedTileDebugLabel = value;
+                    that.picking = true;


This option shouldn't turn picking on.

rahwang · 2017-05-17T18:54:19Z

Thanks for the review, @lilleyse ! This is ready for another look.

As we were discussing, the labels still stick when scene.pickTranslucentDepth = false.

lilleyse · 2017-05-18T14:04:29Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

                            that._pickStatsText = getStats(that._tileset, true);
+


Remove empty line.

lilleyse · 2017-05-18T14:04:59Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                            if (scene.pickPositionSupported && (pos = scene.pickPosition(e.endPosition))) {
+                                that._tileset._debugPickPosition = pos;
+                            }
+                            that._tileset._debugPickedTile = picked.content._tile;


You can now do picked.content.tile (recently added).

lilleyse · 2017-05-18T14:09:38Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

                            that.feature = picked;
+                            var pos;
+                            e.endPosition.x += 10;
+                            e.endPosition.y -= 10;


Instead of offsetting the pick position, you could add a pixelOffset to the label. This may fix some of the other issues, but we'll see.

lilleyse · 2017-05-18T14:11:06Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                            var pos;
+                            e.endPosition.x += 10;
+                            e.endPosition.y -= 10;
+                            if (scene.pickPositionSupported && (pos = scene.pickPosition(e.endPosition))) {


The second statement belongs inside the if block.

lilleyse · 2017-05-18T14:11:25Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

                            that.feature = picked;
+                            var pos;


Avoid abbreviations. position is fine.

lilleyse · 2017-05-18T14:16:08Z

Source/Scene/Cesium3DTileset.js

+            if (defined(tileset._debugPickedTile)) {
+                addTileDebugLabel(tileset._debugPickedTile, tileset);
+                if (defined(tileset._debugPickPosition)) {
+                    tileset._tileDebugLabels._labels[0].position = tileset._debugPickPosition;


Use tileDebugLabels.get.

rahwang · 2017-05-22T15:34:19Z

Thanks for the review, @lilleyse ! The pixelOffset did the trick -- looks like the flickering was from interference from the label.

lilleyse

Getting close. The actual behavior is perfect now.

lilleyse · 2017-05-22T16:21:45Z

Specs/Scene/Cesium3DTilesetSpec.js

@@ -1451,6 +1451,32 @@ defineSuite([
        });
    });

+    it('show only picked tile debug label with all stats', function() {


This test should be setting _onlyPickedTileDebugLabel to true.

You can use expect(scene).toPickAndCall(function(result) { to set _debugPickedTile, and then something else for _debugPickPosition. The scene.pickForSpecs call isn't required.

As part of the same test, set _debugPickedTile to undefined and check that no label is present.

lilleyse · 2017-05-22T16:24:14Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

@@ -325,11 +325,18 @@ define([
                if (value) {
                    that._eventHandler.setInputAction(function(e) {
                        var picked = scene.pick(e.endPosition);
-                        if (picked instanceof Cesium3DTileFeature) {
+                        if (picked && defined(picked.content)) {


Use defined(picked) instead of just picked.

lilleyse · 2017-05-22T17:30:26Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                                position = scene.pickPosition(e.endPosition);
+                                that._tileset._debugPickPosition = position;
+                            }
+                            that._tileset._debugPickedTile = picked.content.tile;


This area could be organized a little differently so that it only does these steps if showOnlyPickedTileDebugLabel is true. Also setting that.feature = picked may have side effects when picked is not a feature.

lilleyse · 2017-05-22T17:33:20Z

Source/Scene/Cesium3DTileset.js

+        var halfAxes = boundingVolume.halfAxes;
+        var radius = boundingVolume.radius;
+
+        var position = Cartesian3.clone(boundingVolume.center, scratchCartesian);


I should have left this comment earlier, but since there are two different ways of positioning labels now, maybe they can be broken out into separate functions.

hpinkos · 2017-05-22T17:39:37Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                            var position;
+                            if (scene.pickPositionSupported) {
+                                position = scene.pickPosition(e.endPosition);
+                                that._tileset._debugPickPosition = position;


Cesium3DTilesInspectorViewModel shouldn't be setting all of these private attributes of Cesium3DTileset. If they can be set without consequences, should they be public instead?

Yeah I wasn't sure about this either or how strict we get for tightly-coupled areas like this. Maybe they should be public but still marked as private for exclusion from doc?

Does it need to be private? We have all kinds of public debug properties in Scene

Hmm, I made them public for now, but I see Sean's point about excluding from docs.

rahwang · 2017-05-23T14:02:14Z

Thanks for the review, @hpinkos and @lilleyse -- comments addressed!

lilleyse · 2017-05-23T14:56:30Z

Source/Scene/Cesium3DTileset.js

@@ -400,6 +402,11 @@ define([
         */
        this.debugShowGeometricError = defaultValue(options.debugShowGeometricError, false);

+        this._tileDebugLabels = undefined;
+        this._onlyPickedTileDebugLabel = false;


For consistency this should lose the underscore as well.

lilleyse · 2017-05-23T14:56:44Z

Source/Scene/Cesium3DTileset.js

+            if (defined(tileset.debugPickedTile)) {
+                var position = (defined(tileset.debugPickPosition)) ? tileset.debugPickPosition : computeTileLabelPosition(tileset.debugPickedTile);
+                addTileDebugLabel(tileset.debugPickedTile, tileset, position);
+                tileset._tileDebugLabels.get(0).pixelOffset = new Cartesian2(15, -15); // Offset to avoid picking the label.


Maybe a bit cleaner if addTileDebugLabel returns the label, then set the pixelOffset on that rather than getting from the collection.

lilleyse · 2017-05-23T14:58:40Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                                that._tileset.debugPickPosition = position;
+                            } else {
+                                position = undefined;
+                            }


No need for this else block.

lilleyse · 2017-05-23T16:53:27Z

When zoomed out I sometimes see situations where a tile is picked but the pick position returns undefined. This causes the tile to move to the corner of the tile sometimes. Probably the best solution is to not set the debugPickPosition if it would be undefined.

rahwang · 2017-05-23T17:23:17Z

What causes pickPosition to return undefined? Are there any scenarios where that would happen outside of !scene.pickPositionSupported?

rahwang · 2017-05-23T17:45:49Z

Fixed, @lilleyse

lilleyse · 2017-05-23T17:49:13Z

What causes pickPosition to return undefined? Are there any scenarios where that would happen outside of !scene.pickPositionSupported?

It can happen if scene.globe.depthTestAgainstTerrain is false (the default) where the terrain doesn't write depth.

lilleyse · 2017-05-24T16:13:29Z

Thanks @rahwang, looks good!

debug label only for picked tile

7457f0a

rahwang force-pushed the pick-tile-debug-label branch from 26a1581 to 7457f0a Compare May 15, 2017 21:18

lilleyse reviewed May 16, 2017

View reviewed changes

rahwang added 2 commits May 17, 2017 14:11

labels follow cursor, fix for no batch table

64a4c6f

Merge branch '3d-tiles' into pick-tile-debug-label

05193ca

lilleyse reviewed May 18, 2017

View reviewed changes

adding pixelOffset

ab2efcb

lilleyse reviewed May 22, 2017

View reviewed changes

hpinkos reviewed May 22, 2017

View reviewed changes

rahwang force-pushed the pick-tile-debug-label branch from 059e5d0 to 670e865 Compare May 23, 2017 13:58

cleanup view model feature picking logic

1a04748

rahwang force-pushed the pick-tile-debug-label branch from 670e865 to 1a04748 Compare May 23, 2017 14:23

lilleyse reviewed May 23, 2017

View reviewed changes

cleanup picking behavior

04d1c4e

lilleyse merged commit 5489bcb into CesiumGS:3d-tiles May 24, 2017

rahwang deleted the pick-tile-debug-label branch June 26, 2017 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only show picked tile debug labels #5325

Only show picked tile debug labels #5325

rahwang commented May 15, 2017

lilleyse left a comment •

edited

Loading

lilleyse May 16, 2017

lilleyse May 16, 2017

lilleyse May 16, 2017

rahwang commented May 17, 2017

lilleyse May 18, 2017

lilleyse May 18, 2017

lilleyse May 18, 2017

lilleyse May 18, 2017

lilleyse May 18, 2017

lilleyse May 18, 2017

rahwang commented May 22, 2017

lilleyse left a comment

lilleyse May 22, 2017

lilleyse May 22, 2017

lilleyse May 22, 2017

lilleyse May 22, 2017

hpinkos May 22, 2017

lilleyse May 22, 2017 •

edited

Loading

hpinkos May 22, 2017

rahwang May 23, 2017

rahwang commented May 23, 2017

lilleyse May 23, 2017

lilleyse May 23, 2017

lilleyse May 23, 2017

lilleyse commented May 23, 2017

rahwang commented May 23, 2017

rahwang commented May 23, 2017

lilleyse commented May 23, 2017

lilleyse commented May 24, 2017

Only show picked tile debug labels #5325

Only show picked tile debug labels #5325

Conversation

rahwang commented May 15, 2017

lilleyse left a comment • edited Loading

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 17, 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 22, 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

lilleyse May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahwang commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented May 23, 2017

rahwang commented May 23, 2017

rahwang commented May 23, 2017

lilleyse commented May 23, 2017

lilleyse commented May 24, 2017

lilleyse left a comment •

edited

Loading

lilleyse May 22, 2017 •

edited

Loading