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

scene.drillPick does not work with Primitive batching #2442

Open
mramato opened this issue Jan 29, 2015 · 10 comments · Fixed by #2443
Open

scene.drillPick does not work with Primitive batching #2442

mramato opened this issue Jan 29, 2015 · 10 comments · Fixed by #2443

Comments

@mramato
Copy link
Contributor

mramato commented Jan 29, 2015

scene.drillPick currently works by picking, hiding the picked primitive, and then repeating the process until no primitives are picked. This means that when using Primitive batching, only one geometry from each primitive gets picked, making dillPick basically useless.

This has been a problem since it was first introducedin #978, but as far as I know, no one realized it until now.

// Create the viewer.
var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;

var defaultColor = Cesium.Color.RED.withAlpha(0.6);
var positions = ([
    -115.0, 37.0,
    -115.0, 32.0,
    -107.0, 33.0,
    -102.0, 31.0,
    -102.0, 35.0
]);

var polygons = [];
for (var i = 0; i < 5; i++) {
    
    polygons.push(new Cesium.GeometryInstance({
        id: 'polygon' + i,
        geometry : Cesium.PolygonGeometry.fromPositions({
            positions : Cesium.Cartesian3.fromDegreesArray(positions.map((p) => p + i)),
            vertexFormat : Cesium.PerInstanceColorAppearance.VERTEX_FORMAT
        }),
        attributes: {
            color: Cesium.ColorGeometryInstanceAttribute.fromColor(defaultColor)
        }
    }));
}

// Add each polygon instance to primitives.
scene.primitives.add(new Cesium.Primitive({
    geometryInstances : polygons,
    appearance : new Cesium.PerInstanceColorAppearance({
        closed : true,
        translucent : true
    })
}));


var pickedPrimitives = [];

function uncolor(picked) {
    picked.forEach(function(picked) {
        var primitive = picked.primitive;
        var color = primitive.getGeometryInstanceAttributes(picked.id).color;
        color[0] = defaultColor.red * 255;
        color[1] = defaultColor.green * 255;
        color[2] = defaultColor.blue * 255;
        color[3] = defaultColor.alpha * 255;
        primitive.getGeometryInstanceAttributes(picked.id).color = color;
    });
}

var altColor = Cesium.Color.YELLOW.withAlpha(0.8);
function setColor(picked) {
    return picked.map(function(picked) {
        var primitive = picked.primitive;
        var color = primitive.getGeometryInstanceAttributes(picked.id).color;
        color[0] = altColor.red * 255;
        color[1] = altColor.green * 255;
        color[2] = altColor.blue * 255;
        color[3] = altColor.alpha * 255;
        primitive.getGeometryInstanceAttributes(picked.id).color = color;
        return picked;
    });
}

var handler = new Cesium.ScreenSpaceEventHandler(scene.canvas);
handler.setInputAction(function(movement) {
    uncolor(pickedPrimitives);
    // get an array of all primitives at the mouse position
    var pickedObjects = scene.drillPick(movement.endPosition);
    if (Cesium.defined(pickedObjects)) {
        pickedPrimitives = setColor(pickedObjects);
    }
}, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 29, 2015

Looks like it is supposed to work, but it requires that per-instance show is used and we need to swap the order of the if statements: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Scene.js#L1607

We could probably just always include that in the shader, but we have bigger fish to fry at the moment. We could keep this Sandcastle example with primitives if needed.

@mramato
Copy link
Contributor Author

mramato commented Jan 29, 2015

Looks like you're right. I was able to fix it with some minor tweaks so that it works as long as there's a show attribute (which the Entity geometry always has). I'll open a PR for it.

mramato added a commit that referenced this issue Jan 29, 2015
Partially fixes #2442 (except in cases where there is no show attribute).
@mramato
Copy link
Contributor Author

mramato commented Jan 29, 2015

I assume we should keep this open since it still fails if the primitive does not have a show instance attribute.

@mramato mramato reopened this Jan 29, 2015
@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 29, 2015

Yes

@mantelpiece
Copy link

This seems to cause an infinite loop if you are not using a ShowGeometryInstanceAttribute on the GeometryInstances and are just showing an entire Primitive using primitive.show.
The typeof primitive.getGeometryInstanceAttributes === 'function' condition is always true, but defined(attributes) && defined(attributes.show) is always false, leading to nothing being hidden and the loops repeats again on the same instance.

@mramato
Copy link
Contributor Author

mramato commented Feb 17, 2015

Good catch. We should just shut off the entire primitive if it doesn't have the show attribute.

EDIT: Good, not Could

mramato added a commit that referenced this issue Feb 17, 2015
As pointed out by @mantelpiece in #2442, if batched Primitives do
not have a show attribute, we end up in an infinite loop because
the primitive is never hid.

This change fixes that problem as well as cleans up drillPick in general
to be a little faster and easier to read.  Also added tests for batched
primitive picking, of which we had none.
@lloyda06
Copy link

I found that its possible for Scene.drillPick to get into an infinite loop. The sandcastle link below uses a KML feed that causes it to occur fairly consistently, but it seems like it can happen anytime there is feature data on the globe. If this is a different bug, i can create a new issue.

The issue can be worked around by providing a limit to drillPick (the 2nd argument) as it will break from the while loop when it hits the limit.

In debugging it seemed to always occur on a label or label collection and the color scratch it was looking for in PickFramebuffer.end() was:

colorScratch.toRgba()
255

Pause the time and set it to the max to display all of the features. Move your mouse around the globe, pan, zoom and eventually everything hangs because drillPick is in an infinite loop.
http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Hello%20World.html&label=Showcases&gist=99c5982f8a87e5a11050f2377bbf7fb5

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 2, 2017

CC #4784

@esparano
Copy link

I seem to have a similar problem to lloyda06. In my case, I was accidentally drillpicking outside of the Cesium canvas (not sure if that's relevant), and drillPick would go into an infinite loop in the same way that lloyda06 described. I was using a typical Cylinder entity and trying to select a point primitive in a point primitive collection underneath several cylinders. DrillPick selected the same cylinder entity over and over, not a label, though.

Providing a limit to drillPick is not an acceptable solution in my case. If I specify a limit less than, say, 5, then it doesn't drill far enough to reach the desired entity. If I specify a limit greater than 5, the app starts to lag significantly.

I was not able to reproduce the issue in SandCastle, unfortunately.

@juburr
Copy link
Contributor

juburr commented Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants