camera.getPickRay exception when Scene is not rendered

[srothst1]

The goal of this pull request is to address the issue with camera.getPickRay outlined in #10125

camera.getPickRay is currently throwing an exception that is not consistent with the rest of our Camera spec when Scene is not rendered. Based on the documentation (see pickEllipsoid, etc.) and various tests that I ran, pick operations are supposed to return undefined if the pick fails.

Currently, after the viewer is updated as follows:

viewer.scene.canvas.style.display = 'none';

I receive this error:

Uncaught DeveloperError: normalized result is not a number
Error
    at new DeveloperError (http://localhost:8080/Source/Core/DeveloperError.js:39:11)
    at Function.Cartesian3.normalize (http://localhost:8080/Source/Core/Cartesian3.js:422:11)
    at getPickRayPerspective (http://localhost:8080/Source/Scene/Camera.js:2922:14)
    at Camera.getPickRay (http://localhost:8080/Source/Scene/Camera.js:2994:12)
    at <anonymous>:7:37
    at HTMLButtonElement.button.onclick (http://localhost:8080/Apps/Sandcastle/Sandcastle-header.js:62:9) (on line 422 of http://localhost:8080/Source/Core/Cartesian3.js)

I am using this sandcastle demo for my testing.

A few lingering TODO items:

• Update CHANGES.md
• Update the Camera spec for getPickRay
• Fix specs that I broke

[cesium-concierge]

Thanks for the pull request @srothst1!

• ❕ There was an error checking the CLA! If this is your first contribution, please send in a Contributor License Agreement.
  • Maintainers, this was the error I ran into while attempting to process the CLA check. Please resolve it to continue CLA checking.
  • You'll need to manually check for a submitted CLA

  Error: The service is currently unavailable.
  

• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.

[srothst1]

The following updates have been made to Camera.prototype.getPickRay in Camera.js:

• Added logic that checks if scene.canvas.style["display"] === "none"
• If scene.canvas.style["display"] === "none", return undefined` given that the scene is not visible.
• This seems more practical than calling getPickRayPerspective or getPickRayOrthographic, which yield runtime errors.

[ggetz]

Thanks @srothst1 ! Before you get too deep in the specs, I have a suggestion about the approach here.

[srothst1]

Hi @ggetz. I am wondering if you have any feedback on the Travis CI build failures. When I run our specs using Jasmine on my local build, I seem to be getting various errors. These errors seem to occur even if I revert my branch back to 918c253, which had no issues when it was committed.

Edit: It seems like the last commit did not fail the Travic CI build. I suspect that the error was related to our above conversation.

[ggetz]

Awesome, looking great @srothst1 ! I just have one small comment.

[ggetz]

I suspect that the error was related to our #10139 (comment).

Yes, eslint was reporting an error and failing CI. You can check for eslint errors locally before you commit by running npm run eslint.

[srothst1]

Hi @ggetz. Please let me know if there is anything else that I need to take care of before this can be merged.

[ggetz]

Looks good, thanks @srothst1 !

[mramato]

This is a breaking change and should probably be listed as such. getPickRay can now return undefined, which was not the case before.

[srothst1]

@ggetz should I open up another PR that fixes CHANGES.md?

[ggetz]

@mramato Even if those cases would have thrown a non-obviously related exception before?

@srothst1 If so, can you please open a PR to update CHANGES.md so that your bullet is under "Breaking changes"?

[srothst1]

Hi @ggetz. Yes, I will open up another PR 👍

[mramato]

Even if those cases would have thrown a non-obviously related exception before?

Without TypeScript, I would agree with you, but with TypeScript, yes. The below TypeScript compiles fine with 1.90 but will fail with main after this PR.

let result: Ray;
const camera = new Camera(new Scene({ canvas: canvasElement }));
result = camera.getPickRay(new Cartesian2(0, 0));

With the error

Specs/TypeScript/index.ts:396:1 - error TS2322: Type 'Ray | undefined' is not assignable to type 'Ray'.

But more philosophically, changing a return type should always be documented as a breaking change because any code calling getPickRay must take into account that undefined is possible, and any code written against the previous documentation will not necessarily have done that.