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

Possible fix for possible scene.pickFromRay bug #7129

Closed
wants to merge 2 commits into from

Conversation

sherousee
Copy link

We encountered an error when testing scene.pickFromRay().

After calling pickFromRay(), the function returns with results, however the scene crashes. We've not been able to reproduce the bug using an unminified version of Cesium, but the exception is functions[i] is not a function. Inspecting functions[i] shows that it is undefined sometimes.

This is a naive approach to fixing this problem, but it does appear to stop the scene from crashing.

@lilleyse

@cesium-concierge
Copy link

Thank you so much for the pull request @sherousee! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

lilleyse commented Oct 9, 2018

I wonder if the case here is pickFromRay is being called from another after-render function, like a tile load event or something similar.

Could you try removing the callAfterRenderFunctions(scene) line in getRayIntersection. I removed this line in the follow-up PR because I was worried about something like this happening.

Line: https://github.com/AnalyticalGraphicsInc/cesium/pull/7115/files#diff-fb63af82386df4a707d9fcfb196c35fcL3692

@sherousee
Copy link
Author

That could make sense, I will give it a shot. Thanks!

@sherousee
Copy link
Author

sherousee commented Oct 12, 2018

@lilleyse Pulling this in from the cesium-dev mailing list where I was having a conversation with @OmarShehata :

Ended up running into another issue after that. 'scene.pickFromRay()' will occasionally return undefined in cases where I know it should actually return a result. From what I can tell, I think the issue is in 'PickFramebuffer.prototype.end' either 'context.readPixels()' is not invoked correctly or the colors are not being indexed correctly from the 'pixels' object. Of course, I could be completely wrong.

@lilleyse
Copy link
Contributor

The picked position from pickDepth.getDepth must also be undefined then if pickFromRay is returning undefined.

A couple questions - is the portion of the tileset being picked on screen? And could you share the code that's calling pickFromRay?

If it helps to compare against something, there's a development demo using pickFromRay here.

@sherousee
Copy link
Author

@lilleyse the picked position from pickDepth.getDepth is actually returning 0 in the cases when pickFromRay is returning undefined.

Yes at the time of picking, the entire tileset is on screen:
image
(base layer hadn't finished loading in that screenshot)
This is an interesting question though. Does the tileset need to be on screen in order for it to be picked?

The code is pretty basic:

    const ray = new Cesium.Ray(image.position, image.direction);
    const result = this.viewer.scene.pickFromRay(ray);

I'm using pickFromRay the same way it is depicted in the development demo you linked.

I am confident the ray calculation is correct because it is exactly the same calculation as I have been using to pick. Using my previous implementation for picking, after all of my data is loaded in the scene, I should have 17 points of intersection with the tileset. Using scene.pickFromRay(), I only have 11. The 11 that do show up are exactly correct. The same 11 show up every time.

While debugging, I tried manually adjusting the ray direction slightly by adding 0.001 to each component of the ray just to see if there were 'holes' in my model that the correct ray passed directly through. This did have an effect. Some of intersections that were previously missing showed up, and some of the rays that previously showed up went missing.

I am not sure exactly how to interpret this. I also don't know exactly how you've implemented the picking. From what I can tell, you are using a color based approach where each triangle/feature/something in the scene is assigned a unique color. The color of a feature is then used as an id. My instinct is that perhaps there are not enough unique ids that can be generated strictly using r,g,b,a for each feature in my tileset to receive a unique id. This would mean some of the features end up with the same id. But again, I really don't know!

@lilleyse
Copy link
Contributor

lilleyse commented Oct 12, 2018

Yeah the tileset does have to be on screen for pickFromRay to work. It also only picks from the level of detail that is visible to the camera, it's not a most-detailed pick. I should probably add that to the pickFromRay documentation (it is part of the sampleHeight and clampToHeight documentation which are the public facing functions):

     * This function only samples height from globe tiles and 3D Tiles that are rendered in the current view. Samples height
     * from all other primitives regardless of their visibility.

Ah and I was mixing up a couple concepts. If pickDepth.getDepth returns 0 then the position will be undefined, and pickFromRay will return undefined. So I don't think it's a bug in the pick id code, but rather just nothing being picked.

Can you give pickFromRayMostDetailed/drillPickFromRayMostDetailed a try? They are implemented in #7115 and will pick the higheset detail tiles on screen or off screen.

@sherousee
Copy link
Author

@lilleyse Awesome, pickFromRayMostDetailed() works great! This is exactly what we need, thank you.

@lilleyse
Copy link
Contributor

@sherousee given that the fix is also in #7115 I'm going to close this. Thanks!

@lilleyse lilleyse closed this Oct 18, 2018
@sherousee
Copy link
Author

Sounds good!

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

Successfully merging this pull request may close these issues.

3 participants